Re: [libmicrohttpd] MHD_add_connection Problems

[Christian Grothoff]

Hi Damon,

You're right, the combination of MHD_NO_LISTEN_SOCKET and MHD internal
threads did not work properly. However, the 'race' you are trying to fix
doesn't really apply. Also,
I think it is a bit safer to test for the existence of the IPC socket
before creating the threads in the no-listen-socket case (as otherwise
we cannot stop the threads). But, that should be a very, very minor detail.

After confirming that it seems to fix the test you provided, I have now
pushed a patch to Git  as revision efd1dd05..1d9f940d. The fix is a
minor variation on your suggestion, so thanks for the report and diagnosis!

Happy hacking!

Christian

On 9/23/20 11:28 PM, Damon Earp wrote:
> Hoping this is as a reply to my previous email.
> 
> I did some digging and below is a patch to daemon.c which allows an
> epoll internal thread to have connections added to it from another
> thread. This patch removes the condition where if MHD_NO_LISTEN_SOCKET
> is present the internal thread isn't started. And it moves setting the
> connection->epoll_state to before calling epoll_ctl avoiding a race
> condition when MHD_add_connection is called from another thread.
> 
> This patch is specific  to epoll_internal w/o turbo and was done ad hoc
> by an ignorant developer. If another type of setup was used this patch
> would probably cause problems.
> 
> Are there other hazards to this? From my gdb tracing it looks like all
> the daemon accesses are synchronized, but I'm interested in getting some
> feedback from someone who actually knows the code and can point me in a
> better direction.
> 
> Thanks,
> Damon
> 
> --- libmicrohttpd-0.9.71/src/microhttpd/daemon.c 2020-06-16
> 18:38:13.000000000 +0000
> +++ libmicrohttpd-0.9.71-patched/src/microhttpd/daemon.c 2020-09-23
> 21:08:52.000000000 +0000
> @@ -2718,11 +2718,13 @@
> 
>        event.events = EPOLLIN | EPOLLOUT | EPOLLPRI | EPOLLET;
>        event.data.ptr = connection;
> +      connection->epoll_state |= MHD_EPOLL_STATE_IN_EPOLL_SET;
>        if (0 != epoll_ctl (daemon->epoll_fd,
>                            EPOLL_CTL_ADD,
>                            client_socket,
>                            &event))
>        {
> +        connection->epoll_state &= ~MHD_EPOLL_STATE_IN_EPOLL_SET;
>          eno = errno;
>  #ifdef HAVE_MESSAGES
>          MHD_DLOG (daemon,
> @@ -6438,8 +6440,7 @@
>    }
>  #endif /* HTTPS_SUPPORT */
>  #if defined(MHD_USE_POSIX_THREADS) || defined(MHD_USE_W32_THREADS)
> -  if ( (0 != (*pflags & MHD_USE_INTERNAL_POLLING_THREAD)) &&
> -       (0 == (*pflags & MHD_USE_NO_LISTEN_SOCKET)) )
> +  if ( (0 != (*pflags & MHD_USE_INTERNAL_POLLING_THREAD)) )
>    {
>      if (0 == daemon->worker_pool_size)
>      {

signature.asc
Description: OpenPGP digital signature