Re: [libmicrohttpd] more on MHD_USE_THREAD_PER_CONNECTION and races

[Christian Grothoff]

Dear Eivind,

I already implemented it (SVN HEAD), but a PM I sent you was killed by your 
spam filter (and I got a reject notice).

-Christian

On Tuesday, June 21, 2011 06:24:53 PM Eivind Sarto wrote:
> 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