Please see my comments below. On 12.08.2021 12:36, Irion, Alexander wrote:
I would say that the timer granularity and the fix are two different things, even if granularity would be finer, the checking condition like before was wrong and would lead to unnecessary busy cycles.
That's correct in part about busy cycles. Thanks for the report.But without changing resolutions of the timer, the proper fix would be changing calculation of the value returned by MHD_get_timeout().
Is a timeout of only one second a real use case?
Three seconds timeout is more realistic use-case. With this patch it is changed to 2-3 seconds. The condition "greater" (instead of "greater or equal") was used by purpose: with limited granularity this is the only way to make connection expire *after* defined timeout value, not "after or just before" (as with this patch).
In any way, your example below would result in a returned timeout value of 1000ms, so you would sleep 1s into the external poll, before calling MHD_run() again. So, in fact it would be closed at 51.999. However, yes - when a new event would come in shortly after 50.999 this would trigger the connection close earlier.
That's the case. If more than one connection is served by the same server, external poll could be triggered much earlier than one second. If many connections are served the chances are quite high that something happen within 1/100 of second which trigger MHD_run() which trigger immediate connection expiration with this patch.
If user sets timeout value to 10 second, user expects that connection would not be closed before 10 seconds of inactivity. As activity newer happen at xx.0000 moments, connection will expire (almost) always before defined timeout (if more than one connection is served) with this patch.
-- Evgeny
The report itself is correct, thanks for this! However while the patch fixed one problem, it creates another one. The problem connected to low granularity (rounded to one second) of the MHD's internal timer. If connection timeout is set to one second (for example) then connection may expire at any moment up to one second with the patch applied. A full example: if activity is recorded at 50.999 seconds, which is rounded to 50 internally, then connection with 1 second timeout can be closed at 51.0, which is not what is expected by user. The proper solution would be increase of the resolution of the internal MHD timer. I'll handle it. -- Evgeny On 11.08.2021 23:39, Christian Grothoff wrote:Thanks for the detailed report, the patch looks good as-is, applied to master as f5d387df7e7fb8de1a5dd0739ddb83a8b19fe64b Happy hacking! Christian On 8/11/21 6:02 PM, Irion, Alexander wrote:Hello! I discovered a problem with closing connections, when MHD_OPTION_CONNECTION_TIMEOUT is used. When I set the timeout option to 3 seconds and connect with a browser, I get a timeout value from MHD_get_timeout() of 3000 ms returned. I then sleep the desired time and call MHD_run again(). Then, MHD_get_timeout() returns repetitive a timeout of 0 which causes thousands of calls to MHD_run(), taking up several hundred milliseconds and burning CPU time. I use an external event loop with MHD_DAEMON_INFO_EPOLL_FD. As reason I found that the condition timeout < (MHD_monotonic_sec_counter () - connection->last_activity) will not be true, after waiting the timeout time. The right side will be equal to timeout, so the connection is not closed, MHD_get_timeout() continues to return a timeout value of 0, and this runs repetitively in a loop for some thousand iterations, until the time jumps to the next second. In the attachment please find a small patch for fixing this issue. It's just turning the < into a <=.----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
OpenPGP_signature
Description: OpenPGP digital signature
