Re: [libmicrohttpd] upgrading and life cycle of sockets, issue when used with epoll

[José Bollo]

Hello Christian,

Thank you for the advise. I had not thought about that before but there
is surely a need for a side channel notification in my case. I have
to check...

The dream for me that it could be blindly integrated in LMHD translates
perhaps to a nightmare for you... I let you mind about that, extend or
not the TODO list, and return to my code.

Thank you again for your work.

Best regards
José

On Wed, 6 Jan 2021 20:45:21 +0100
Christian Grothoff <grothoff@gnunet.org> wrote:

> Hi Jose,
> 
> Yes, we should indeed return a timeout of 0 in this case. I've
> implemented that in df6124f7..b1691240.
> 
> However, please note that I would still strongly advise the use of a
> signalling channel, as your
> approach only works in limited cases where your
> closing of the connection is done within the scope of the MHD_run()
> operation. The moment you actually do such operations later based on
> other things in your external event loop, you will need some kind of
> signalling.
> 
> Happy hacking!
> 
> Christian
> 
> On 1/5/21 10:17 AM, José Bollo wrote:
> > On Sun, 3 Jan 2021 14:22:25 +0100
> > Christian Grothoff <grothoff@gnunet.org> wrote:
> >   
> >> Hi Jose,
> >>
> >> I've figured out the issue.  The bug is actually in your code.  
> > 
> > Hi Christian,
> > 
> > Thank you for having checked that issue.
> >   
> >> The problem is that you are using an _external_ event loop, and if
> >> you do so, you are responsible for re-triggering MHD_run() if you
> >> do anything that might change the state of an MHD connection.
> >> This (mostly) applies to MHD_resume_connection() -- but also to
> >> closing an upgraded connection.
> >>
> >> Without this modification, MHD does close the connection on the
> >> _next_ request, so if you would connect with another client, MHD
> >> will then close the connection.  
> > 
> > Yes, the next request closes the connection and unlocks the first
> > client. But this is not a solution, obviously.
> >   
> >> This is the only way to fix this, as when MHD is not 'run', it
> >> cannot act. And if you are controlling the external event loop,
> >> you are responsible for triggering MHD 'when necessary'.
> >>
> >> I've attached a modified version of your code that adds the
> >> necessary signalling logic to your poll()-loop. (Using eventfd();
> >> non-portable. Use a pipe() to do this in a more portable way.)  
> > 
> > I have checked your rewrite. (A side note, reformatting code is not
> > fair when using tools for a quick search of differences.)
> > 
> > Before accepting your conclusion, I'd like to argument a little the
> > reason why I'm still thinking that something can be done in your
> > code.
> > 
> > The main loop i submitted is:
> > 
> >   while (poll(pfd, 1, -1) == 1) {
> >     do {
> >       MHD_run(d);
> >     }
> >     while(
> >        MHD_get_timeout(d, &to) == MHD_YES
> >        && to == 0);
> >   }
> > 
> > The close is done in a user callback invoked within MHD_run. Then
> > in my opinion, the function MHD_get_timeout should return a timeout
> > of zero because there is no time to wait before the next action:
> > closing something.
> > 
> > If you accept that idea, it simplifies my code and probably avoid
> > further questions if someone also integrates an external polling
> > mechanism.
> > 
> > Otherwise, lets go for handling specifically the squared corner
> > case of the wheel.
> > 
> > Hopefully I convinced you.
> > Best regards
> > José
> >   
> >>
> >>
> >> Happy hacking!
> >>
> >> Christian
> >>
> >> On 12/10/20 4:08 PM, José Bollo wrote:  
> >>> Hello,
> >>>
> >>> My code uses LMHD embedded with its EPOLL mechanism. Part of that
> >>> code deals with upgrading to websocket. It then call somewhere:
> >>>
> >>>   response = MHD_create_response_for_upgrade(
> >>>                upgrade_to_websocket, memo);
> >>>
> >>> and the callback function upgrade_to_websocket looks as here
> >>> below:
> >>>
> >>>   void upgrade_to_websocket(
> >>>              void *cls,
> >>>              struct MHD_Connection *connection,
> >>>              void *con_cls,
> >>>              const char *extra_in,
> >>>              size_t extra_in_size,
> >>>              MHD_socket sock,
> >>>              struct MHD_UpgradeResponseHandle *urh
> >>>   ) {
> >>>       struct memo *memo = cls;
> >>>       struct ws *ws = ws_create(sock, memo, close_websocket, urh);
> >>>       if (ws == NULL) close_websocket(urh);
> >>>   }
> >>>
> >>>   void close_websocket(struct MHD_UpgradeResponseHandle *urh) {
> >>>       MHD_upgrade_action (urh, MHD_UPGRADE_ACTION_CLOSE);
> >>>   }
> >>>
> >>> Thank you for your attention until here. So far, so good.
> >>>
> >>> The issue now: when the functiuon ws_create returns NULL, the
> >>> program returns to some polling and wait for an events BUT DOES
> >>> NOT CLOSE THE SOCKET, leading to starvation of the client.
> >>>
> >>> I guess that calling some function after calling
> >>> MHD_upgrade_action (urh, MHD_UPGRADE_ACTION_CLOSE) could unlock
> >>> the situation by performing correct close. Though the called
> >>> function should not be MHD_run because it dispatch events, what
> >>> is not expected here.
> >>>
> >>> I join a sample demo. When I connect on websocket on it, the
> >>> client starves. I recorded and joined the output of strace.
> >>>
> >>> Best regards
> >>> José Bollo
> >>>
> >>>  
> >   
>