Re: [libmicrohttpd] more on MHD_USE_THREAD_PER_CONNECTION and races

[Piotr Grzybowski]

hello.

 i think that there is no point in closing a connection that is
refferenced by something, so
some protection would be nice. red-black tree indexed by connection fd
would be nice,
where each connection has a refcount and when refcount goes to 0
connection is no longer
used and can be closed. and certainly no point in calling close on
connections which have
refcount>0.
 i am not sure about the acutall race here, but it seems possible.

sincerely,
pg


On Tue, Jun 21, 2011 at 2:39 AM, Eivind Sarto <address@hidden> wrote:
> I have some additional info on races in the library when using 
> MHD_USE_THREAD_PER_CONNECTION.
>
> It looks like MHD_handle_connection() races against MHD_cleanup_connections() 
> after MHD_connection_close() gets called.
> The problem is with the main loop in MHD_handle_connection():
>
> MHD_handle_connection(void *data)
> {
>  struct MHD_Connection *con = data;
>
>  while ((!con->daemon->shutdown) && (con->socket_fd != -1)) {
>       /* whenever MHD_connection_close() is called from within this loop (or 
> from any place within
>        * the state machine, there is a race with MHD_cleanup_connections().
>        */
>  }
> }
>
> The problem is that the while-loop will come back and reference the 
> connection structure, but after a MHD_connection_close() the
> MHD daemon may have freed the connection structure.
>
>
> Maybe this is a good time to also fix a big performance issue with using MHD 
> with a lot of connections.
> MHD_cleanup_connections() scans all the connections after a new connection 
> has been accepted.
> If you have 1000s of connections that are keep-alive, you will be scanning 
> 1000s of connections to find that one connection
> that have been closed.
>
> Maybe it would be a better idea to implement an "active" connection list and 
> a "closed" connection list (both doubly linked lists).
> Protect them with a mutex.  After a connection has been closed and  the 
> connection thread is completely done referencing the structute
> (like at the end of MHD_handle_connection), it grabs the lock and moves the 
> connection structure from the "active" to the "closed" connection list.
> The MHD_cleanup_connections() also grabs the lock and only cleans up 
> connections on the "closed" list.
> No race, and much less overhead with lots of connections.
>
> What do you think?
>
> -eivind
>
>
>