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

[José Bollo]

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
> > 
> >