[GNUnet-SVN] r34385 - libmicrohttpd/src/microhttpd

[gnunet]

Author: grothoff
Date: 2014-10-28 09:55:50 +0100 (Tue, 28 Oct 2014)
New Revision: 34385

Modified:
   libmicrohttpd/src/microhttpd/daemon.c
Log:
Milan is right.

>> Nevertheless, this means that there is an unhandled special case.
>> Consider MHD_USE_THREAD_PER_CONNECTION (either with or without
>> MHD_USE_POLL). Then wpipe is not created. After MHD_quiesce_daemon,
>> socket_fd = -1. Then, after the current select/poll in the
>> MHD_select_thread exits, there will be no fds to wait for (the socket_fd
>> is -1 and wpipe was not created), so the MHD_select_thread will be
>> busy-waiting for daemon->shutdown. Therefore, another condition should
>> be added to the beginning of MHD_quiesce_daemon:
>> current:
>>
>>   MHD_quiesce_daemon (struct MHD_Daemon *daemon)
>>   {                                                                          
>>                                                                              
>>                                  
>>     unsigned int i;                                                          
>>                                                                              
>>                                  
>>     int ret;                                                                 
>>                                                                              
>>                                  
>>                                                                              
>>                                                                              
>>                                  
>>     ret = daemon->socket_fd;                                                 
>>                                                                              
>>                                  
>>     if (-1 == ret)                                                           
>>                                                                              
>>                                  
>>       return -1;                                                             
>>                                                                              
>>                                  
>>     if ( (-1 == daemon->wpipe[1]) &&                                         
>>                                                                              
>>                                  
>>          (0 != (daemon->options & MHD_USE_SELECT_INTERNALLY)) )              
>>                                                                              
>>                                  
>>     {                                                                        
>>                                                                              
>>                                  
>> #if HAVE_MESSAGES                                                            
>>                                                                              
>>                                  
>>       MHD_DLOG (daemon,                                                      
>>                                                                              
>>                                  
>>                 "Using MHD_quiesce_daemon in this mode requires 
>> MHD_USE_PIPE_FOR_SHUTDOWN\n");                                               
>>                                               
>> #endif                                                                       
>>                                                                              
>>                                  
>>       return -1;
>>     }
>>
>> new:
>>
>>     if ( (-1 == daemon->wpipe[1]) &&
>>          (0 != (daemon->options & (MHD_USE_SELECT_INTERNALLY | 
>> MHD_USE_THREAD_PER_CONNECTION )) ))
>>
>> Did I get it right?
>
> I don't think so.  Note that the 'socket_fd' in the thread's
> select()/poll() call is not the (closed) listen socket, but the thread's
> TCP connection to the client.  So the thread can still be terminated by
> calling shutdown() on that TCP connection socket.
> (The code is a bit confusing here, as both structs have a member called
> 'socket_fd').

(I am sorry I cannot make myself clear enough.)

I still think what I wrote holds. I am talking about the thread
executing MHD_select_thread, i.e., the one created in MHD_start_daemon_va.
It gets the daemon structure as parameter and accesses daemon->socket_fd.
I believe you are talking about threads executing MHD_handle_connection,
created in internal_add_connection.

The problem is that the listening thread might run out of FDs to wait
for. In the MHD_USE_THREAD_PER_CONNECTION case, the main listening
thread only selects/polls on two FDs -- daemon->socket_fd, and
daemon->wpipe[0]. And when daemon->socket_fd is closed and
daemon->wpipe[0] does not exist, problem occurs.

The same issue could happen when there is a thread_pool, but that is
taken care of in MHD_quiesce_daemon -- in this case, if there is no
wpipe, MHD_quiesce_daemon fails. What I am suggesting that this test
should also include the MHD_USE_THREAD_PER_CONNECTION case.

Cheers,
Milan Straka




Modified: libmicrohttpd/src/microhttpd/daemon.c
===================================================================
--- libmicrohttpd/src/microhttpd/daemon.c       2014-10-27 20:35:53 UTC (rev 
34384)
+++ libmicrohttpd/src/microhttpd/daemon.c       2014-10-28 08:55:50 UTC (rev 
34385)
@@ -2773,7 +2773,7 @@
   if (MHD_INVALID_SOCKET == ret)
     return MHD_INVALID_SOCKET;
   if ( (MHD_INVALID_PIPE_ == daemon->wpipe[1]) &&
-       (0 != (daemon->options & MHD_USE_SELECT_INTERNALLY)) )
+       (0 != (daemon->options & (MHD_USE_SELECT_INTERNALLY | 
MHD_USE_THREAD_PER_CONNECTION))) )
     {
 #if HAVE_MESSAGES
       MHD_DLOG (daemon,