Fix reply not sent when event loop terminates prematurely #13492

1. 
   promag commented at 1:50 am on June 18, 2018: member

   Fixes #11777. Closes #13485.

   When calling RPC stop, there is a race when the event loop terminates before HTTPRequest::WriteReply and so the reply event fails to process. This happens because event_base_dispatch returns when there are no active or pending events, which is the case between InterruptHTTPServer and HTTPRequest::WriteReply.

2. http: Fix reply not sent when event loop terminates prematurely 74aced681f
3. fanquake added the label RPC/REST/ZMQ on Jun 18, 2018
4. 
   promag commented at 1:51 am on June 18, 2018: member

   @ken2812221 and @MarcoFalke please review.
5. 
   promag commented at 1:57 am on June 18, 2018: member

   By the way, this is only possible with libevent 2.1.8, so if this is accepted we have to change the minimum version

   0| libevent | [2.1.8-stable](https://github.com/libevent/libevent/releases) | 2.0.22 | No |  |  |
   

6. 
   ken2812221 commented at 2:37 am on June 18, 2018: contributor

   How about we add an infinity timeout timer event before event_base_dispatch and delete it before event_base_loopexit? This might works with older version libevent. (Haven’t tested)
7. 
   promag commented at 8:47 am on June 18, 2018: member

   @ken2812221 unless there is a strong reason to not bump libevent, that could work too. In that case, it should be enough to remove the timer (instead of event_base_loopexit) and then the event loop would terminate.
8. fanquake requested review from laanwj on Jun 18, 2018
9. 
   MarcoFalke commented at 11:56 am on June 18, 2018: member

   I am not sure what the exact policy for bumping the minimum version of a dependency is, but we should at least compile on Xenial, which doesn’t come with 2.1.8.
10. 
    promag commented at 12:07 pm on June 18, 2018: member

    Thanks @MarcoFalke, in that case I’ll update with an alternative. WDYT about keeping this for 2.1.8 and provide an alternative for <2.1.8?
11. 
    fanquake commented at 12:10 pm on June 18, 2018: member

    Thanks @MarcoFalke, in that case I’ll update with an alternative. WDYT about keeping this for 2.1.8 and provide an alternative for <2.1.8?

    That should be fine, we already do that elsewhere in the code IIRC.

12. 
    laanwj commented at 3:07 pm on June 18, 2018: member

    Concept ACK.

    Thanks @MarcoFalke, in that case I’ll update with an alternative. WDYT about keeping this for 2.1.8 and provide an alternative for <2.1.8?

    Yes, please do that. Especially the BSDs tend to come with ancient versions of libevent, so please don’t change the minimum requirement but put this in #ifdefs.

13. 
    MarcoFalke commented at 3:15 pm on June 18, 2018: member

    WDYT about keeping this for 2.1.8 and provide an alternative for <2.1.8?

    If the alternative has no downsides, I’d rather say just do the alternative and keep the code more concise and well tested. (note that travis and most developers don’t run the old libevent versions)

14. 
    promag commented at 5:05 pm on June 19, 2018: member

    Better fix in #13501.
15. promag closed this on Jun 19, 2018

    ---

16. DrahtBot locked this on Sep 8, 2021

Contributors
promag ken2812221 MarcoFalke fanquake laanwj

Review Requested
laanwj

Labels
RPC/REST/ZMQ