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

[Christian Grothoff]

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

signature.asc
Description: OpenPGP digital signature