Re: [libmicrohttpd] suspend/resume patch

[José Bollo]

Le mercredi 20 avril 2016 à 20:41 +0300, Evgeny Grin a écrit :
> 
> On 20.04.2016 19:27, Flavio Ceolin wrote:
> > 
> > Any other thoughts about this ? Should I consider this change not
> > wanted
> > ?

Hi all,

I'd like to recall you that I'd like to see this change integrated.

> Please note, that connection in suspended mode still counted for IP
> limits and pre-daemon limit, but will not timeout.
> You changes are not perfectly thread-safe. If you don't know
> connection
> suspended/resumed state, then MHD_resume_connection() could return
> but
> connection will be suspended (as state may be changed by other thread
> during execution of MHD_resume_connection() in this thread).

The issues in suspend/resume are improtant to be known but are not
related to the fact that the upgraded connection is closed.

The fact that resouces like IP limits are counted in is safe and I
agree with that.

I really don't care when the resume is taken into account because when
resume is called, libmicrohttpd handles the socket again for any of its
purpose (including closing it). This is the natural semantic. I tried
to close the socket before but moved quickly backward because
libmicrohttpd stops the program in that case. That is a little bit
violent but it can be understood.

The only issue that I see is minor: on receiving an upgrade, there is a
risk that the connection will not be closed. What is the really risk? 

First case: the upgrade is refused by the HTTP server, if the client
try to use the upgraded protocol it will raise an error and the
connection will be closed sooner or later. This issue is related to a
client error.

Second case: the upgrade is accepted but no action take place to handle
the new protocol. Then the client will use the new protocol that will
soon raise an error. It is an error in the server integration. It will
sooner or later close the connection.

Handling the new protocol currently means to make suspend/resume but it
may change in some future.

Third case: nothing more happen on the connection and here yes, I can
imagine a leak. But it is IMHO the only case.

Please could check again whether the following patch that avoid to
close the connection when upgrade is called is valid or not.

Best regards
José Bollo

diff -Naur a/src/microhttpd/connection.c b/src/microhttpd/connection.c
--- a/src/microhttpd/connection.c       2016-04-08 19:02:26.000000000
+0000
+++ b/src/microhttpd/connection.c       2016-04-08 19:02:26.000000000
+0000
@@ -731,8 +731,7 @@
   {
     if (NULL == end)
       return MHD_YES;
-    if ( (MHD_str_equal_caseless_ (end, "close")) ||
-         (MHD_str_equal_caseless_ (end, "upgrade")) )
+    if ( (MHD_str_equal_caseless_ (end, "close")) )
       return MHD_NO;
    return MHD_YES;
   }