Re: [libmicrohttpd] more on MHD_USE_THREAD_PER_CONNECTION and races

[Christian Grothoff]

Hi Eivind,

I'm already working on a patch along those lines.  You're certainly right 
about the race and the O(n) which could be O(1). I have a patch that adds a 
doubly-linked list for both 'active' and 'cleanup' connections and I think we 
only need a lock around the 'cleanup' list.  However, there is still some bugs 
left in the code.  I'll probably commit something later today...

Piotr: we don't need to add reference-counting, there is always only one 
entity "responsible" for a connection; the problem is that that entity cannot 
always to the full cleanup since a pthread cannot call 'pthread_join' on 
itself ;-).  

Happy hacking,

Christian

On Tuesday, June 21, 2011 02:39:01 AM Eivind Sarto 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