Re: [libmicrohttpd] more on MHD_USE_THREAD_PER_CONNECTION and races

[Eivind Sarto]

That is great that you will have some changes.  My test can cause a 
failure/race in less than a minute, so I can validate the
changes.

If you go with the 'active' and 'cleanup' lists, you would definitely need to 
lock around both list if the connection thread is the
is moving the connection from 'active' to 'cleanup'.  I hope that is the 
approach you are taking since that will also solve the
performance issue I see when the daemon is scanning the current 'connections' 
list for a closed socket.

I am processing more than a 1000 connections per seconds and the cpu load goes 
sky high when the daemon is scanning
5000 connections every time an accept happens.

Just let me know when you commit something and I'll give it a try.

-eivind
________________________________________
From: address@hidden address@hidden On Behalf Of Christian Grothoff 
address@hidden
Sent: Tuesday, June 21, 2011 4:53 AM
To: address@hidden
Cc: Eivind Sarto
Subject: Re: [libmicrohttpd] more on MHD_USE_THREAD_PER_CONNECTION and races

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