Project

General

Profile

[Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library

Added by birdofprey about 1 year ago

Hey folks, I am new here, and I am trying to embed Lighttp server into an Android app. With minor code upgrades I managed to turn it into a shared library and bridge it with Java code via JNI, and it seems to work, pretty cool! However, I have encountered a problem: when I shutdown the server some connections remain "ESTABLISHED", and it causes loosing some request when if I later restart the server on the same origin. I guess, I might be doing something not correct, but actually after some troubleshooting I believe, there might be a little bug in the lighttpd code.

In my understanding (looking at code of release v1.4.68), during the server shutdown the call to connections_free() (line #2098 of server.c) is supposed to close established connections, and looking what it calls, it seems the function connection_reset() (line #553 of connections.c) is ultimately responsible for closing the connection, however it never closes a file descriptor associated with the connection. If I add the line "close(con->fd);" to that function, then it seems to work as expected. And I believe, I am able to see the difference on Linux, if I start the server with simple config and -D flag, request a test asset, then close the server and immediately check connections status with "netstat -t" in my console: without that close() added I see some connection remains "ESTABLISHED" for some time after the server shutdown, and with added close() it seems to close immediately.

What do you think, does it make sense, or did I get everything wrong?


Replies (33)

RE: Connections remain "ESTABLISHED" after server shutdown - Added by gstrauss about 1 year ago

In my understanding (looking at code of release v1.4.68), during the server shutdown the call to connections_free() (line #2098 of server.c) is supposed to close established connections, and looking what it calls, it seems the function connection_reset() (line #553 of connections.c) is ultimately responsible for closing the connection, however it never closes a file descriptor associated with the connection.

Incorrect. connections_free() never calls close(), so I do not understand how you concluded that it should.
connections_free() is responsible for freeing the memory associated with struct connection

The aptly-named connection_close() calls close() on a connection. connection_close() is separate from connections_free(). This is not a mistake.

.

When the server is gracefully (cleanly) shutting down, the server waits until all remaining connections have closed before shutting down.

When the server is not gracefully shutting down, the server does not wait until all remaining connections (if any) have closed, and exiting the program closes the descriptors. In some extreme cases (such as benchmarking) where a lot of connections may be open, calling close() on thousands of file descriptors takes a measurable amount of time longer than simply exiting the program and letting the kernel handle closing the open fds.

It is clear that you are trying to use lighttpd code for a purpose for which the lighttpd code was not intended, so what you have identified is not a bug.

What you are currently doing sounds like an unclean lighttpd shutdown, and you are asking in the unclean shutdown case that lighttpd cleanly close any remaining open fds rather than exiting as quickly as possibly for the unclean shutdown case (and let the kernel close the remaining open fds).

Why are you shutting down uncleanly when there are open connections? I do not have any visibility into your app or usage, but if I were you, I would look into closing those connections cleanly rather than an abrupt close(), especially when protocol-specific mechanisms exist for a clean connection shutdown, such as HTTP/2 GOAWAY, or TLS alert CLOSE_NOTIFY.

I am not necessarily against adjusting lighttpd's behavior on unclean shutdown, but I would like to understand more before considering any potential changes.

RE: Connections remain "ESTABLISHED" after server shutdown - Added by birdofprey about 1 year ago

Hi @gstrauss, thanks for rapid reply. As you noticed, I don't have a clear understanding how it supposed to work, and I am bruteforcing my way through with some old good trial-and-error approach.

My aim is to embed Lighttpd inside an Android app, and in my current understanding Android is not quite friendly to bundling-in and running stand-alone executables as a part of an app. It prefers shared libraries. So my current idea is quite simple: I got Lighttpd code, I updated CMakeLists to build it as a shared library, instead of an executable, and I went into server.c and slightly updated beginning of main() function turning it into a JNI-friendly function Java_com_lighttpd_Server_launch(), which can be called from Java layer passing in arguments which normally come from the command line, then it maps those arguments to argc and argv, and then it is all original code, with just a couple simple tweaks, running the server as it usually would and seemingly working fine.

Now, my desire is to be able to shutdown and start the server again. For that I added a new function Java_com_lighttpd_Server_shutdown(), which just sets graceful_shutdown=1; srv_shutdown=1;, as I figured out these global variables cause the server to shutdown in the original code, and my expectation were that during shutdown the server cleans-up / resets it state, so when I later call Java_com_lighttpd_Server_launch() with the same config, it just restarts at the same origin like the first time. However, as I said, I figured out that when I do it some connections stay established, and then when I restart the server and send in a request usually the first (few) requests land to that old connection and remain pending, while future requests cause a new connection to open and that seem to work fine.

At this point, I started to look what server does on shutdown, and figured out that it never closes connections, and I just confirmed that adding that close() to connections_free() does close them (also at some point while troubleshooting I removed graceful_shutdown flag from my stop function, and now I am just setting srv_shutdown to stop it).

Now, if I got your message correctly, the expectation is that on non-graceful shutdown the system closes connections when the program exits, but that probably does not work for my scenario of shared library, where the program does not exit when I stop the server. It sounds I should add explicit connections_close() calls to the server shutdown flow? Am I getting something wrong here? Is connections_close() call blocking, which will delay the server shutdown?

RE: Connections remain "ESTABLISHED" after server shutdown - Added by gstrauss about 1 year ago

Your second posted restated your first post without adding much to the conversation. Did you read my post? You did not answer any of my questions.
You managed to pick up that I highlighted the difference between "clean" and "unclean" shutdowns. Then you did not structure your post around that distinction.
You have yet to revisit your guess at setting graceful_shutdown=1; srv_shutdown=1;. That is incorrect for a graceful shutdown.
Did you look what happens in the signal handler in server.c when lighttpd gets SIGINT?

RE: Connections remain "ESTABLISHED" after server shutdown - Added by birdofprey about 1 year ago

Sorry, I read your post, and I thought my message answers your question " Why are you shutting down uncleanly when there are open connections? "

You managed to pick up that I highlighted the difference between "clean" and "unclean" shutdowns. Then you did not structure your post around that distinction.

Ok, I guess I need graceful shutdown.

when protocol-specific mechanisms exist for a clean connection shutdown, such as HTTP/2 GOAWAY, or TLS alert CLOSE_NOTIFY.

I know nothing about it. I'd expect, I ask server to shutdown gracefully, and then server should take care about the exact mechanisms.

Did you look what happens in the signal handler in server.c when lighttpd gets SIGINT?

Yes, I tried, I found this code:

case SIGINT:
    if (graceful_shutdown) {
        if (2 == graceful_restart)
            graceful_restart = 1;
        else
            srv_shutdown = 1;
    } else {
        graceful_shutdown = 1;
    }
    break;

and this looks to me just setting those graceful_shutdown and srv_shutdown flags, and leaving the server's main loop to react on it. Do I miss something here?

RE: Connections remain "ESTABLISHED" after server shutdown - Added by birdofprey about 1 year ago

Ogh... I see, it should be just graceful_shutdown=1, and setting srv_shutdown = 1; causes non-graceful shutdown, and SIGINT handler sets it if SIGINT arrives once graceful shutdown is already underway.

RE: Connections remain "ESTABLISHED" after server shutdown - Added by gstrauss about 1 year ago

Do I miss something here?

Um. Yes? I really shouldn't have to point out that there are branching conditions in the handful of lines of code you posted above. The first time lighttpd receives SIGINT, graceful_shutdown = 0 (default state at lighttpd server startup) and lighttpd sets graceful_shutdown = 1

If you're wrapping server.c into a library, you're obviously using lighttpd in a way that was not intended and you need to reset globals if you reuse the code without unloading and then reloading the shared library.

RE: Connections remain "ESTABLISHED" after server shutdown - Added by gstrauss about 1 year ago

Ok, I guess I need graceful shutdown.

I do not know your app (nor do I need to know the specifics), but thank you for clarifying what you are actually trying to do.

when protocol-specific mechanisms exist for a clean connection shutdown, such as HTTP/2 GOAWAY, or TLS alert CLOSE_NOTIFY.

I know nothing about it. I'd expect, I ask server to shutdown gracefully, and then server should take care about the exact mechanisms.

Yes, for a graceful shutdown.

However, to avoid hanging around too long during graceful restart or graceful shutdown,
lighttpd graceful shutdown/restart additionally provides a mechanism to close connections after a timeout.
See server.feature-flags "server.graceful-shutdown-timeout" => 8

Edit: reminder: do not overlook what I posted above for server.c file-scoped globals:

you need to reset globals if you reuse the code without unloading and then reloading the shared library.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by birdofprey about 1 year ago

Thanks for help @gstrauss!

you need to reset globals if you reuse the code without unloading and then reloading the shared library.

Yes, thanks, in theory have this understanding; in practice, I had a brief look at code and I am under impression that most of globals are already reset when the server is started, and I am pretty sure I might have overlooked some that are not... similar to my mistake with the shutdown logic ¯\_(ツ)_/¯

One more question, as I already explained what I am trying to achieve, would be there any interest on your side to merge in my changes to support bulding Lighttpd as a shared JNI-friendly library? If you have a moment to have a look, here is the diff of what I did vs v1.4.68:
https://github.com/birdofpreyru/lighttpd1.4/compare/6f6837d07698296b294ee1aff6ae89cbef2c1db8...birdofpreyru:lighttpd1.4:535aee47b15216e268473dad4e174b5c715d6539
It builds shared JNI library when cmake is done with BUILD_JNI_LIB flag set, otherwise it build the standard standalone Lighttpd. The result currently seems working on Android; and as I believe, I am not the first person who asks how to make it work with Android, this might be useful for other folks beside me.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

One more question, as I already explained what I am trying to achieve, would be there any interest on your side to merge in my changes to support bulding Lighttpd as a shared JNI-friendly library?

I would consider it. I took a brief look at your code. These notes are not exhaustive.
  • collecting code:
    - All the JNI-wrapping code in server.c could be collected into exactly one place for #include and multiple supporting funcs, plus the one additional callout right before server_main_loop()
    - I would prefer to isolate the library code into its own routine which calls a (renamed) main(). That would also be the place to set optind = 1
  • library:
    - It is generally considered rude for a library to muck with signal handling or other global process state.
    Should the library mode disable or modify lighttpd signal handling? What about other process-wide global state set by lighttpd (e.g. setlocale())? ...other?
  • logging:
    - Is your intention to use Android NDK methods for logging in addition to lighttpd logging (duplicating the log trace)?
    Or should the Android NDK logging replace syslog() in log.c and in mod_accesslog.c (and syslog setup in configfile.c)?
    (Also, this is Android-specific, not Java-specific (BUILD_JNI_LIB))
    ...Perhaps the library interface could define local functions for openlog(), syslog(), and closelog() if a new build option is set (BUILD_ANDROID_NDK_LOGGING?).
  • build:
    - lighttpd supports autoconf, CMake, SCONS, and meson. Your patches are only for CMake (and could use some polishing)
    While extending to all build systems is not an absolute requirement, it is something I have to consider for consistency and for maintenance.
    - BUILD_JNI_LIB assumes a) Linux and b) Android. Java and lighttpd both run on many more platforms.
    The CMake code should at least document the Linux- and Android-specific code.
  • code patterns:
    - lighttpd does not use '//' except potentially in third-party code
    - your patch contains some unnecessary whitespace changes (CMakeLists.txt and plugin.c) and mismatched whitespace (plugin.c, LighttpdMacros.cmake)
    - my pattern for using preprocessor directives differs from the one used in your patch

Edit: bug: argv[] should have final entry of NULL. This is not C++ or Java, so argv[6] is not guaranteed to be 0-initialized. Declaring a new argv[] on the stack shadows the original argv and should result in a warning with modern compilers using -Wall. It would be cleaner if you created a new array jniargv[] and then assigned argv = jniargv;

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by birdofprey about 1 year ago

Hey @gstrauss, when you have a moment, will you have another look, please:
https://github.com/birdofpreyru/lighttpd1.4/compare/6f6837d07698296b294ee1aff6ae89cbef2c1db8...birdofpreyru:lighttpd1.4:38b97d34272dfdd2c7881b66826866cb467ef7c1

I did a clean-up of my changes, following your remarks.

It is generally considered rude for a library to muck with signal handling or other global process state.
Should the library mode disable or modify lighttpd signal handling? What about other process-wide global state set by lighttpd (e.g. setlocale())? ...other?

I don't know, and lack knowledge about processes. The way I currently use it for Android, I haven't seen any side-effects of signal handling by Lighttpd. I launch the server in a new Thread on Java side, and it does not seem Java itself sends in any interruption signals when I call .interrupt() for that Thread, that's why I added dedicated Java_com_lighttpd_Server_shutdown() routine which can be called via JNI to stop the server. Then, my naive guess is that Thread runs as a dedicated process, and thus any signal handling Lighttpd does is isolated from the rest of app running in other threads, but I might be totally wrong.

Is your intention to use Android NDK methods for logging in addition to lighttpd logging (duplicating the log trace)?
Or should the Android NDK logging replace syslog() in log.c and in mod_accesslog.c (and syslog setup in configfile.c)?

Not sure, and lack knowledge about best logging practices on Linux / Android. The way it works now seems good to me: __android_log_print() from NDK sends log messages into Java logs, the ones can be viewed via adb logcat tool. Now, only the most important events logged via log_error_write() (like server start / shutdown / grave errors) are send there, giving a nice idea about high-level server events relative to other stuff happening in an app. Then, if I enable debug logging in Lighttpd config, that still outputs to a dedicated file in the app's document folder, and it has both messages logged via log_error_write(), as well as other debug info, and that's nice to have in that separate file, which can be looked through for details of server behavior, without polluting app-wide Java logs with too much server-specific info.

I also introduced a separate build flag BUILD_ANDROID_NDK_LOGGING allowing to do JNI build with or without NDK logging bit.

- lighttpd supports autoconf, CMake, SCONS, and meson. Your patches are only for CMake (and could use some polishing)

While extending to all build systems is not an absolute requirement, it is something I have to consider for consistency and for maintenance.

Unfortunately, I have zero knowledge of autoconf, SCONS, and meson, and not much motivation to learn them right now, so can't do that piece.

The CMake code should at least document the Linux- and Android-specific code.

Not sure I get it, do you want # comments in CMakeLists explaining what these new flags are for? I don't see much comments in CMakeLists regarding existing options, thus I wonder do you mean some other kind of documenting?

argv[] should have final entry of NULL.

Thanks, I just did not know standards say the last argv[] value should be NULL, I always thought naively that as argc specifies the number of arguments, argv[] can be just of argc length, and if it is larger any other elements aren't important.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

Thank you for taking the time to incorporate my feedback.

I have some more questions and some concerns.

To incorporate this into lighttpd, I would like to do so in a maintainable and portable fashion. I am not an expert in Java, and from your post above, I am not sure you are the most qualified person to help me integrate a generic shared library solution for use with JNI. You have a method which works for your use case, but in your own words, you "lack knowledge about processes".

From what I see, the patch can be segmented into four parts
- build system integration
- wrapping lighttpd main()
- callback before server_main_loop()
- Android NDK logging

The only required part that I see is wrapping lighttpd main(), plus some level of build integration.
The callback before server_main_loop() is convenient for you, but you have not shared why it is important or necessary. Why?
The logging is convenient for you, but why should I add that part of the patch to lighttpd?
The build system integration: potential alternative: might (might) be cleaner if you renamed main() and included all the code in a single shared library instead of lighttpd and lighttpd_common. Is there a reason that lighttpd and lighttpd_common are required to be separate? Sure, we know that the modules do not need (and should not call) the functions from the server (which are not part of COMMON_SRC), but can those files otherwise co-exist safely in a single shared library? That said, lighttpd_common (or similar) is created on some platforms (like macOS and on Cygwin/Windows) where linking with a separate library is necessary, so it might make sense to keep lighttpd_common as a more generic solution.

Smaller, more isolated patches will be easier to maintain external to lighttpd, especially if I choose not to integrate one or more of those pieces that you find convenient, but which I might not consider "generic" enough to be included.

Why is the following change needed? Is there something I was doing incorrectly in the CMakeLists.txt? Is this an improvement? (I am not a CMake expert)

-set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_SOURCE_DIR}/COPYING")
-set(CPACK_RESOURCE_FILE_README "${CMAKE_SOURCE_DIR}/README")
+set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_SOURCE_DIR}/COPYING")
+set(CPACK_RESOURCE_FILE_README "${CMAKE_CURRENT_SOURCE_DIR}/README")

I understand that -Wl,-no-undefined" is needed for your use case, but that linker flag is specific to gcc and clang, and might not be supported by other compiler-linker toolchains.

The Android logging might be a standalone patch (which I might not include).
Here is some untested, isolated code which could go into server.c to provide local definitions to redirect syslog() to Android NDK logging.
This would also intercept mod_accesslog iff mod_accesslog were configured to use syslog.

#ifdef BUILD_ANDROID_NDK_LOGGING                                                         
#ifdef HAVE_SYSLOG_H

#include <stdarg.h>
#include <syslog.h>
#include <android/log.h>                                                                           

static const char *android_log_tag = "lighttpd";                                                   

void openlog(const char *ident, int option, int facility)                                          
{                                                                                                  
    android_log_tag = ident;/*(configfile.c passes persistent constant string)*/                   
    UNUSED(option);                                                                                
    UNUSED(facility);
}               

void closelog(void)                                                                                
{           
}           

void syslog(int priority, const char *format, ...)
{           
    switch (priority) {
      case LOG_EMERG:   priority = ANDROID_LOG_FATAL; break;                                       
      case LOG_ALERT:   priority = ANDROID_LOG_FATAL; break;                                       
      case LOG_CRIT:    priority = ANDROID_LOG_ERROR; break;                                       
      case LOG_ERR:     priority = ANDROID_LOG_ERROR; break;                                       
      case LOG_WARNING: priority = ANDROID_LOG_WARN;  break;                                       
      case LOG_NOTICE:  priority = ANDROID_LOG_INFO;  break;                                       
      case LOG_INFO:    priority = ANDROID_LOG_INFO;  break;                                       
      case LOG_DEBUG:   priority = ANDROID_LOG_DEBUG; break;                                       
      default:          priority = ANDROID_LOG_ERROR; break;                                       
    }   

    va_list ap;
    va_start(ap, format);
    __android_log_vprint(priority, android_log_tag, format, ap);                                   
    va_end(ap);
}       

#endif /* HAVE_SYSLOG_H */                                                                         
#endif /* BUILD_ANDROID_NDK_LOGGING */                                                             

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by birdofprey about 1 year ago

I am not sure you are the most qualified person to help me integrate a generic shared library solution for use with JNI

I agree. I'd also be happy if anybody with Java/JNI experience helped with Lighttpd integration.

The callback before server_main_loop() is convenient for you, but you have not shared why it is important or necessary. Why?

To know when the server is bound to an address and it is safe to send in requests.

The logging is convenient for you, but why should I add that part of the patch to lighttpd?

Anybody who develops for Android expects to see device logs via adb logcat command, that's where OS, all apps and libraries send their logs, and that's where NDK logging routines send them. As I wrote in my previous msg, it still feels handy to have detailed Lighttpd debug logs output into an error.log file, but adb logcat is the first stop to understand how a library (Lighttpd in our case) interact with the rest of an app.

Is there a reason that lighttpd and lighttpd_common are required to be separate?

I just tried to keep it as close as possible to your standalone executable build. I am not CMake expert either, thus motto is If this works, don't try to further fix it.

Why is the following change needed? Is there something I was doing incorrectly in the CMakeLists.txt? Is this an improvement? (I am not a CMake expert)
-set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_SOURCE_DIR}/COPYING")
-set(CPACK_RESOURCE_FILE_README "${CMAKE_SOURCE_DIR}/README")
+set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_SOURCE_DIR}/COPYING")
+set(CPACK_RESOURCE_FILE_README "${CMAKE_CURRENT_SOURCE_DIR}/README")

Yeah, once you start to integrate the library into Android project relying on native C/C++ code, there will be another "primary" CMakeLists.txt which orchestrate the build of all C/C++ pieces, one of which will be Lighttpd. When CMake runs for that combined project CMAKE_SOURCE_DIR variable holds the URL of that "primary" CMakeLists.txt location, and then Lighttpd build fails not able to locate those COPYING and README files. CMAKE_CURRENT_SOURCE_DIR points to the folder containing the current CMakeList.txt being processed, thus in that combined project it will point to Lighttpd location (and in a standalone build these variables just hold the same value when the root CMakeList.txt of Lighttpd is processed).

I understand that -Wl,-no-undefined" is needed for your use case, but that linker flag is specific to gcc and clang, and might not be supported by other compiler-linker toolchains.

Yeah, I don't know. It is just when I ran Android app builds, that flag is automatically included and the app build fails if something isn't correctly linked. Thus, I added it so that when I run Lighttpd JNI build separately the build behavior matches what I'll see when I run the build within the host Android project.

The Android logging might be a standalone patch (which I might not include).

This looks cool, thanks, I'll give a try to your version of NDK logging later.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

I've tried to be clear that I am asking questions for reasons of generic reuse, consistency, and maintenance.

The callback before server_main_loop() is convenient for you, but you have not shared why it is important or necessary. Why?

To know when the server is bound to an address and it is safe to send in requests.

I already know that. I already know that is convenient for you. That's fine. You have not answered the question why it might be important or necessary in a generic situation.

The lighttpd test suite (external to the lighttpd executable) tests for readiness by testing if the test suite can connect to the expected listening socket. Your solution is not useful for the lighttpd test suite. Since I do not know your app, I asked: why is the callback important or necessary? Why is this the generic solution? (It is not.) Why does your app need to know this information? (Some apps do not need this information -- they just spawn a thread and then go about their business.)

Yes, I fully agree that a callback is convenient when you are waiting to do something. However, that is not the question I am asking.
Is your method a typical idiom in Java? Would you point to some other examples? FYI: a condition variable is another generic means to achieve notification, and is a low-level construct that is portable across many languages. Alternatively, I could set a file-scoped global around server_main_loop() and you can write a loop which checks the value in a spin loop. That would not be the best solution for most, but might be okay for your use case (probably not). I don't know. That's why I am asking questions. Alternatively, I could tell you to test for socket readiness. A callback uses fewer resources, but this is only at server startup. ... Actually, it is not only at server startup, but also after graceful_restart, and while your application might not care and might not use graceful_restart, some other application might.

Let me try to phrase this differently: you have a solution which works for you, and that is excellent. You asked me if I would include some/all of it in lighttpd. My questions are all related to inclusion of something generically reusable in lighttpd. Otherwise, your patches continue to be great for you and you should use your patches for you.

At the moment, your callback for server readiness is convenient for you, but I judge it as not currently implemented as a generic solution to include in the lighttpd code. You'll have to maintain that as a separate patch if it is required for your application.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

The android logging patch is small and direct. Maybe it should be wrapped in something like BUILD_ANDROID_NDK_DUP_LOG_ERROR and be your original patch which only duplicates the error trace from log.c. It should be possible to do that without modification to the build files. CFLAGS=-Dfoo before configuring to pass additional defines to the build system without the need for a new build option to simply set the define. (Or a small code patch to enable the behavior.)

-Wl,-no-undefined can be construed as "Android-specific" rather than JNI and so should be wrapped in a check for the Android platform. It should additionally be wrapped in a check for building JNI shared libs if that should apply only when building JNI shared libs.

Separately, here's what I had mocked up (untested) for server.c.
This passes JNIEnv *env to lighttpd_jni_main(), but does not include the call to the callback.

#ifdef BUILD_JNI_LIB

#include <jni.h>

/** Orders graceful server shutdown in JNI mode */
__attribute_cold__
JNIEXPORT void JNICALL Java_com_lighttpd_Server_shutdown(
    JNIEnv *jenv,
    jobject thisObject
) {
    UNUSED(jenv);
    UNUSED(thisObject);
    graceful_shutdown = 1;
}

/* intercept main() when building as JNI library */
__attribute_cold__
int lighttpd_jni_main (int argc, char ** argv, JNIEnv *jenv);
#define main(a,b) lighttpd_jni_main (int argc, char ** argv, JNIEnv *jenv)

/** Entry point for server build in JNI mode */
__attribute_cold__
JNIEXPORT jint JNICALL Java_com_lighttpd_Server_launch(
    JNIEnv *jenv,
    jobject thisObject,
    jstring configPath
) {
    UNUSED(thisObject);

    const char *config_path = (*jenv)->GetStringUTFChars(jenv, configPath, 0);
    if (!config_path) return -1; /*(lighttpd will fail without config)*/

    optind = 1;
    char *argv[] = { "lighttpd", "-D", "-f", config_path, NULL };
    int rc = lighttpd_jni_main(4, argv, jenv);

    (*jenv)->ReleaseStringUTFChars(jenv, configPath, config_path);
    return rc;
}
#endif /* BUILD_JNI_LIB */

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

This (untested) would replace all of your code modifications to lighttpd *.[ch] files, except the optional change to plugin.c, which might be needed for the Android build (not the generic JNI build) for Android to find shared libs in the APK. (untested) This code also highlights that you made some assumptions in your code for your specific use (which are fine), but which are not generic and are not documented as requirements, e.g. required methods for callbacks.

Isolating this patch to be as least invasive as possible should allow you to use this patch and hopefully avoid conflicts with changes to lighttpd source code in the future. Absent advice and guidance from an expert in Java and JNI and C integration, I do not currently see this being accepted into lighttpd source code, as I do not have the expertise myself to validate any part of this as a generic solution which might be useful to others. You have explained some of the pieces and why they are useful to you, and I have no doubt those might be useful to others, too.

#ifdef BUILD_ANDROID_NDK_SYSLOG_INTERCEPT
#ifdef HAVE_SYSLOG_H

#include <stdarg.h>
#include <syslog.h>
#include <android/log.h>

static const char *android_log_tag = "lighttpd";

void openlog(const char *ident, int option, int facility)
{
    android_log_tag = ident;/*(configfile.c passes persistent constant string)*/
    UNUSED(option);
    UNUSED(facility);
}

void closelog(void)
{
}

void syslog(int priority, const char *format, ...)
{
    switch (priority) {
      case LOG_EMERG:   priority = ANDROID_LOG_FATAL; break;
      case LOG_ALERT:   priority = ANDROID_LOG_FATAL; break;
      case LOG_CRIT:    priority = ANDROID_LOG_ERROR; break;
      case LOG_ERR:     priority = ANDROID_LOG_ERROR; break;
      case LOG_WARNING: priority = ANDROID_LOG_WARN;  break;
      case LOG_NOTICE:  priority = ANDROID_LOG_INFO;  break;
      case LOG_INFO:    priority = ANDROID_LOG_INFO;  break;
      case LOG_DEBUG:   priority = ANDROID_LOG_DEBUG; break;
      default:          priority = ANDROID_LOG_ERROR; break;
    }

    va_list ap;
    va_start(ap, format);
    __android_log_vprint(priority, android_log_tag, format, ap);
    va_end(ap);
}

#endif /* HAVE_SYSLOG_H */
#endif /* BUILD_ANDROID_NDK_SYSLOG_INTERCEPT */

/* Note: This code to build lighttpd as a JNI library must be located:
 * - after server_main_loop() is defined
 * - before main() is defined
 */
#ifdef BUILD_JNI_LIBWRAP

#include <jni.h>

/* JNI library build: intercept main() and pass in (JNIEnv *) */
__attribute_cold__
int lighttpd_jni_main (int argc, char ** argv, JNIEnv *jenv);
#define main(a,b) lighttpd_jni_main (int argc, char ** argv, JNIEnv *jenv)

/* JNI library build: intercept server_main_loop() and pass in (JNIEnv *) */
static void lighttpd_jni_main_loop (server * const srv, JNIEnv *jenv)
{
    /*(callback is optional and might not exist) */
    /*(code should check class and callback exist; !sample code not tested!)*/
    /* JNI library build: callback to signal that lighttpd is up and running */
    jclass ServerClass = (*jenv)->FindClass(jenv, "com/lighttpd/Server");
    if (ServerClass) {
        jmethodID onLaunchedID =
          (*jenv)->GetStaticMethodID(jenv, ServerClass,
                                     "onLaunchedCallback", "()V");
        if (onLaunchedID)
            (*jenv)->CallStaticVoidMethod(jenv, ServerClass, onLaunchedID);
    }

    server_main_loop(srv);
}
#define server_main_loop(srv) lighttpd_jni_main_loop((srv), jenv);

/** JNI library build: entry point */
__attribute_cold__
JNIEXPORT jint JNICALL Java_com_lighttpd_Server_launch(
    JNIEnv *jenv,
    jobject thisObject,
    jstring configPath
) {
    UNUSED(thisObject);

    const char *config_path = (*jenv)->GetStringUTFChars(jenv, configPath, 0);
    if (!config_path) return -1; /*(lighttpd will fail without config)*/

    optind = 1;
    char *argv[] = { "lighttpd", "-D", "-f", config_path, NULL };
    int rc = lighttpd_jni_main(4, argv, jenv);

    (*jenv)->ReleaseStringUTFChars(jenv, configPath, config_path);
    return rc;
}

/** JNI library build: trigger graceful server shutdown */
/*(This would be better-named if method ended with "_graceful_shutdown")*/
__attribute_cold__
JNIEXPORT void JNICALL Java_com_lighttpd_Server_shutdown(
    JNIEnv *jenv,
    jobject thisObject
) {
    UNUSED(jenv);
    UNUSED(thisObject);
    graceful_shutdown = 1;
}

#endif /* BUILD_JNI_LIBWRAP */

Minimizing the patch to CMake or other build options is not done above.

If you remove your fork on github, I hope that you'll attach a patch to this post for others to possibly use.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

For CMakeLists.txt on Android, ANDROID_ABI is required when building with Android_NDK, so I think that you should be able to use

if(ANDROID_ABI)
    [...]
endif()

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

-Wl,-no-undefined should be used only when linking lighttpd modules (shared libs) with lighttpd_common or similar, and probably a good idea to limit that to inside the CMakeLists.txt condition if(CMAKE_C_COMPILER_ID MATCHES "GNU" OR CMAKE_C_COMPILER_ID MATCHES "Clang") (a few lines above your patch) to have a better chance of using a linker which supports the flag.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by birdofprey about 1 year ago

Thanks again! I'll test and adopt your isolated version of the patch in some days. I am currently shifting my focus to integrating Lighttpd into an iOS app (I am actually working on integrating it into a React Native library https://www.npmjs.com/package/@dr.pogodin/react-native-static-server, which I and some other folks using in some apps; and thus my plan is to do a basic integration into that library for iOS, then for Windows, and then come back to revisit and enhance the Android case, which for now already works for me).

#define main(a,b) lighttpd_jni_main (int argc, char ** argv, JNIEnv *jenv)

This is neat! I did not know preprocessor can be used this way to rename functions defined elsewhere.

If you remove your fork on github, I hope that you'll attach a patch to this post for others to possibly use.

No, I'll definitely keep the fork.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

Future potential direction: instead of modifying the build system to change 'lighttpd' from an executable to a library, you may want to leave that as-is and to instead create a new target to build 'lighttpd-jni' as a single shared library (not 'lighttpd' and 'lighttpd-common'). Related, building the lighttpd modules with -Wl,-no-undefined is not required on Linux, though I am not familiar if Android NDK adds requirements. 'lighttpd-jni' still needs to be loaded prior to lighttpd dlopen() of lighttpd modules depending on those symbols, but that has to happen anyway since plugin.c in 'lighttpd-jni' is what calls dlopen() for lighttpd modules.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

Anybody who develops for Android expects to see device logs via adb logcat command, that's where OS, all apps and libraries send their logs, and that's where NDK logging routines send them.

BTW, this sounds to me like a development or debug option, not something which necessarily should be enabled in production use.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by birdofprey about 1 year ago

Hi @gstrauss, I feel like sharing an update on my progress here. This is the code I currently ended up with:
https://github.com/birdofpreyru/lighttpd1.4/compare/6f6837d07698296b294ee1aff6ae89cbef2c1db8...birdofpreyru:lighttpd1.4:e7809c39829203fb60f604dcf12567e6e134b9cd
and it works fine for me, both for Android and iOS.

For Android I now run CMake with arguments -DWITH_ANDROID_NDK_SYSLOG_INTERCEPT=ON -DWITH_JAVA_NATIVE_INTERFACE=ON -DBUILD_STATIC=ON, and it builds for me a shared library as a single file, with selected modules bundled in. The syslog intercept you suggested works great, all cool.

For iOS, although I saw an article about building lighttpd for iOS as an executable, using autoconfig, I wanted to stick to CMake, at least for now. So, I am also following the receipt from CMake documentation (https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-for-ios-tvos-or-watchos) and running CMake config and build as this, which builds it as a static library:

cmake ${PODS_TARGET_SRCROOT} -B ${TARGET_TEMP_DIR} \
        -DCMAKE_OSX_DEPLOYMENT_TARGET=${IPHONEOS_DEPLOYMENT_TARGET} \
        -DCMAKE_SYSTEM_NAME=iOS \
        -DBUILD_STATIC=1 \
        -DBUILD_LIBRARY=1 \
        -GXcode
cmake --build ${TARGET_TEMP_DIR} --config ${CONFIGURATION} \
        --target pcre2-8-static mod_indexfile mod_dirlisting mod_staticfile \
          lighttpd

Here I encountered and corrected a few issues you may want to cherry-pick, to correct cross-compiling for iOS on MacOS using CMake:
  • It was missing to define HAVE_SENDFILE_BROKEN, and it is really broken, at least for me on iOS if sendfile() is called, the app just crashes right away. I also had to update src/network_write.c to wrap all related definitions inside #ifndef HAVE_SENDFILE_BROKEN guard; otherwise it was guarding only for the first definition block, but one of following ones was still configuring the code to rely on sendfile() even with HAVE_SENDFILE_BROKEN defined.
  • As mentioned in that other article on building for iOS, iPhone SDK misses netinet/tcp_fsm.h file, so I added CMake test for the file, and if not found, I just added a generic copy into codebase (though, it looks like lighttpd only uses a one or two constants from that file, probably no need to depend on it at all?).
  • I also found that compilation of lemon with host compiler inside a cross-compilation build was failing for me when doing it for iOS, if I don't explicitly add to that compile command a bunch of arguments specifying host SDK, architecture, include and link paths (otherwise, by default, it was picking selections for target system). Here I am not sure, probably there is a simpler way to workaround such problem?

I also have one question. For now I am just ensuring in a host code that no more than one lighttpd server instance can be launched inside an app at the same time. Because when I run it as a library, those global variables used inside server.c won't allow running multiple server instances. Though, no for right now, but for future, I wonder how feasible it will be to upgrade the code to allow multiple server instances running when using as a library. I imagine, moving those global variables inside server object should not be that difficult. Can you give me just a hint, are there many more dependencies on global state in other files and modules; or does it look a feasible idea to you?

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

Thanks for the update. I'll take a look in the next few days. I do not have an iOS device to test on, and it sounds like the CMake build does not handle HAVE_SNEDFILE_BROKEN. (FYI: lighttpd works find with autotools on Mac OSX.)

Regarding multiple instances, file-scoped globals are used in more than a few places for singletons. One feature of logging is that if NULL is passed as the error handle, it uses the global srv->errh saved at startup. In other words, it would not be trivial to move the global state of the lighttpd application for your use in multiple instances in a library. To be clear, I have no plans to invest any time in this, and any patches proposed would be evaluated against the performance hit in the lighttpd application.

Instead, I recommend that you reconfigure and restart the embedded lighttpd application to listen on multiple ports. Except in very high-traffic cases a single thread of lighttpd can handle quite a bit of load. In other words, instead of spawning a new thread, you would reconfigure and restart the lighttpd thread in your application.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

What header file on iOS defines TCPS_CLOSE_WAIT? BTW, the answer to your questions about "better ways" your patch is to use preprocessor directives in that one location to detect building on iOS and to include the right header for iOS, or simply to disable that feature on iOS by not including <netinet/tcp_fsm.h on iOS.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

library-wrap.h is a good idea to collect your additions to be included in server.c and wrap main(), but would probably be better named jni-library-wrap.h os something JNI-specific.

As I step through the patches, the rest of the patch set is a bit messy of things you tried and later undid.

It looks like you undid the patch to plugin.c to not use a full path to the plugins. I believe you did that for Android. Is that no longer needed? Or did you undo that for iOS, and need to add the logic back for Android, protected by preprocessor options for Android builds (__ANDROID__)?

HAVE_BROKEN_SENDFILE was added for Linux 2.4 where -- according to the comment in the code -- sendfile did not support large files. (I have not verified that.) You decided to reuse HAVE_BROKEN_SENDFILE. Why is sendfile() broken on iOS? "I have unexplained crashes with sendfile() enabled" does point to an issue, but is not the same as "the sendfile() interface is incompatible with how we want to use it with large files". iOS and sendfile() need further troubleshooting before I would accept a patch for this. If it were broken, there should be a bug filed with Apple for providing something that is broken. If lighttpd needs to use sendfile() differently on iOS, then that is something different. I do not know what is wrong with lighttpd use of sendfile() on iOS, but I am not willing to conclude that sendfile() is broken on iOS. You can set lighttpd.conf server.network-backend = "writev" to use writev() instead of sendfile() if you want to do so on iOS.

The build of lemon.c using COMMAND cc should have used the native compiler unless your build environment has done something like override the PATH. Instead of adding ${LEMON_BUILD_FLAGS} -- which is not a great name for "specify the native compiler" -- you probably want to create something like ${NATIVE_CC} which is defined to "cc" and you can override it for iOS. Is VERBATIM required, or just your typical standard practice? Is COMMAND_EXPAND_LISTS required? COMMAND_EXPAND_LISTS is new in cmake 3.8 and lighttpd current minimum version is cmake 3.7.0. See the top-level CMakeLists.txt in lighttpd source tree.

Your workaround for missing <netinet/tcp_fsm.h> could have been better contained as the following. I think we can do better, but this is quick:

#if (defined(__APPLE__) && defined(__MACH__))

#ifdef __has_include
#if __has_include(<netinet/tcp_fsm.h>)
#include <netinet/tcp_fsm.h>
#endif
#endif
#ifndef TCPS_CLOSE_WAIT
#define TCPS_CLOSE_WAIT 5
#endif

#else

#include <netinet/tcp_fsm.h>

#endif

Is __MACH__ defined on iOS? (I have not checked). I had thought it was for Mac OS. I think we can do better than the above by using iOS preprocessor directives and figuring out where TCPS_CLOSE_WAIT is defined in the iOS system headers. The __has_include() has been in clang for a very, very long time, so it is safe to assume it is available when building for iOS.

RE: [Solved] Connections remain "ESTABLISHED" after server shutdown in JNI-wrapped library - Added by gstrauss about 1 year ago

https://github.com/ndfred/iperf-ios/issues/17 suggests some issues with sendfile() on iOS.

I have said this before: lighttpd supports multiple build systems. Your CMake-specific patches are less likely to be accepted.
To avoid using sendfile() in network_write.c, you could add something like

#ifdef some_ios_preprocessor_directive_to_indicate_ios
#undef NETWORK_WRITE_USE_SENDFILE
#endif

below the other preprocessor directives which set NETWORK_WRITE_USE_SENDFILE. That would be simpler patch than what you did.

(1-25/33)