Re: [libmicrohttpd] 0.9.71+ Connection Idle and Reuse Issue when Suspending and Resuming Connections

[Christian Grothoff]

Hi Damon,

The problem is that your logic queues a response during the *first*
callback to 'access_handler', and your 'count' protection there is not
working as intended by you.

You need to be a bit careful with the API, the
MHD_OPTION_NOTIF_CONNECTIONCATION is per socket, while what you need for
_that_ counter is MHD_URI_LOG_CALLBACK and MHD_OPTION_NOTIFY_COMPLETED.
We _know_ that some of our names are confusing for historic reasons, and
I hope that if we ever find funding for a "MHD v2.0" we can make the API
_much_ more intuitive.

Anyway, I've attached a revision of your code that fixes the bad
behavior. Basically: make sure not to queue a response "early" (as per
line connection.c:4014), and MHD will be happy to do pipelining. (This
is not really a regression, more of a bugfix, as before this could
result in really bad unwarranted behaviors --- and we always said that
you should ONLY queue 'early' *if* the client requested 100 CONTINUE and
you want to _abort_ the upload. And in that case, we _must_ force a
"Connection: close").

Happy hacking!

Christian

On 1/5/21 6:32 PM, Earp, Damon N. (GSFC-619.0)[SCIENCE SYSTEMS AND
APPLICATIONS INC] via libmicrohttpd wrote:
> Hey all,
> We're witnessing an increase in one time use connections when testing against 
> 0.9.71 and 0.9.72. It appears to be related to suspending a connection 
> immediately then resuming it in a different thread. We are using Linux 
> (Debian 9.13 amd64). I wrote up a minimal test program and attempted to test 
> connection reuse and the connection idle timeout. Below are the results of 
> each test when running with the old and new versions of the library and the 
> test source code.
> 
> For the tests:
> Version 0.9.62 is provided via aptitude and installed under /usr/lib/.
> Version 0.9.71 is custom compiled without providing any arguments to 
> `./configure` and is located in /usr/local/lib. 
> 
> Thanks for all your hard work. Hopefully there is a small fix for this.
> Damon Earp
> 
> --------------------------------------------------------------------------------
> TEST 1 - Connection Reuse
> Client Command: `$ curl -v localhost:8080{,,,,,,,,,,,,,,}`
> 
> libmicrohttpd 0.9.62
> ```
> $ ./idle
> close after 0.0046 seconds, completed 15 requests
> ```
> 
> libmicrohttpd 0.9.71
> ```
> $ LD_LIBRARY_PATH=/usr/local/lib ./idle 
> close after 0.0004 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> close after 0.0001 seconds, completed 1 requests
> close after 0.0002 seconds, completed 1 requests
> ```
> 
> --------------------------------------------------------------------------------
> TEST 2 - Idle Connection Timeout of 5 Seconds.
> Client Command: `$ perl -e '$|=1;print "GET /index.html 
> HTTP/1.1\r\n\r\n";while(<>){}' | nc localhost 8080`
> 
> libmicrohttpd 0.9.62
> ```
> $ ./idle 
> close after 5.6223 seconds, completed 1 requests
> ```
> 
> libmicrohttpd 0.9.71
> ```
> $ LD_LIBRARY_PATH=/usr/local/lib ./idle 
> close after 0.0027 seconds, completed 1 requests
> ```
> 
> --------------------------------------------------------------------------------
> Test Program
> Build command: `gcc -Wall -Werror -o idle main.c -lmicrohttpd -pthread`
> 
> ```
> #include <microhttpd.h>
> #include <pthread.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <arpa/inet.h>
> #include <sys/socket.h>
> #include <time.h>
> #include <unistd.h>
> 
> #if MHD_VERSION < 0x00097000 
> #define MHD_RESULT_TYPE int
> #else
> #define MHD_RESULT_TYPE enum MHD_Result
> #endif
> 
> struct connection {
>     struct timespec begin;
>     unsigned count;  
> };
> 
> static void *worker(void *userdata)
> {
>     struct MHD_Connection *con = userdata;
>     MHD_resume_connection(con); 
>     return NULL;
> }
> 
> static void connection_notify(void* cls, struct MHD_Connection* con,
>     void** ctx, enum MHD_ConnectionNotificationCode code)
> {
>     if (code == MHD_CONNECTION_NOTIFY_STARTED)
>     {
>         struct connection *c = calloc(sizeof(*c), 1);
>         clock_gettime(CLOCK_MONOTONIC, &c->begin);
>         *ctx = c; 
>     }
>     else if (code == MHD_CONNECTION_NOTIFY_CLOSED)
>     {
>         struct connection *c = *ctx;
>         struct timespec now, diff;
>         clock_gettime(CLOCK_MONOTONIC, &now);
>         diff.tv_sec = now.tv_sec - c->begin.tv_sec;
>         if ((diff.tv_nsec = now.tv_nsec - c->begin.tv_nsec) < 0)
>         {
>             --diff.tv_sec;
>             diff.tv_nsec += 1000000000;
>         }
>         double secs = (diff.tv_sec * 1000000000 + diff.tv_nsec) / 
> 1000000000.0; 
>         printf("close after %.4lf seconds, completed %u requests\n", secs, 
> c->count);
>         free(c);
>     }
> }
> 
> static MHD_RESULT_TYPE access_handler(void *cls, struct MHD_Connection *con,
>     const char *url, const char *method, const char *version,
>     const char *upload_data, size_t *upload_data_size, void **con_cls)
> {
>     /*
>      * hackish trick to detect if this is the first or second time the 
> handler 
>      * has been called for a request. 
>      */
>     uintptr_t *count = (void*) con_cls;
>     if ((*count)++ == 0)
>     {
>         ((struct connection*) MHD_get_connection_info(con, 
> MHD_CONNECTION_INFO_SOCKET_CONTEXT)->socket_context)->count++;
>         MHD_suspend_connection(con);
>         pthread_t t;
>         pthread_create(&t, NULL, &worker, con);
>         pthread_detach(t);
>     }
>     else
>     {
>         struct MHD_Response *resp = MHD_create_response_from_buffer(0, NULL, 
> MHD_RESPMEM_PERSISTENT);
>         MHD_queue_response(con, 200, resp);
>         MHD_destroy_response(resp);
>     }
>     return MHD_YES;
> }
> 
> int main(int argc, const char *argv[])
> {
>     int flags = MHD_USE_EPOLL_INTERNAL_THREAD | MHD_USE_ITC | 
> MHD_ALLOW_SUSPEND_RESUME;
>     struct MHD_Daemon *d = MHD_start_daemon(flags, 8080, NULL, NULL, 
>         &access_handler, NULL, 
>         MHD_OPTION_NOTIFY_CONNECTION, &connection_notify, NULL,
>         MHD_OPTION_CONNECTION_TIMEOUT, 5u,
>         MHD_OPTION_END);
>     pause();
>     MHD_stop_daemon(d);
>     return 0;
> }
> ```
>

test.c
Description: Text Data

signature.asc
Description: OpenPGP digital signature