rpc: work-around an upstream libevent bug #11593

1. 
   theuni commented at 8:23 pm on November 1, 2017: member

   A rare race condition may trigger while awaiting the body of a message.

   This may fix some reported rpc hangs/crashes.

   This work-around mimics what libevent does internally once a write has started, which is what usually happens, but not always due to the processing happening on a different thread: https://github.com/libevent/libevent/blob/e7ff4ef2b4fc950a765008c18e74281cdb5e7668/http.c#L373

   Fixed upstream at: https://github.com/libevent/libevent/commit/5ff8eb26371c4dc56f384b2de35bea2d87814779

2. laanwj added the label RPC/REST/ZMQ on Nov 1, 2017
3. laanwj added this to the milestone 0.15.0.2 on Nov 1, 2017
4. laanwj added the label Needs backport on Nov 1, 2017
5. rpc: work-around an upstream libevent bug

   A rare race condition may trigger while awaiting the body of a message, see
   upsteam commit 5ff8eb26371c4dc56f384b2de35bea2d87814779 for details.
   
   This may fix some reported rpc hangs/crashes.

   6b58360f9b
6. theuni force-pushed on Nov 1, 2017
7. 
   gmaxwell commented at 10:00 pm on November 1, 2017: contributor

   utACK.
8. 
   achow101 commented at 11:52 pm on November 1, 2017: member

   utACK
9. dcousens approved
10. 
    dcousens commented at 1:16 am on November 2, 2017: contributor

    LGTM
11. 
    promag commented at 9:33 am on November 2, 2017: member

    utACK 6b58360.

    For http_set_gencb there is no need to enable/disable since the reply is sent right away in the callback: https://github.com/bitcoin/bitcoin/blob/1b8c88451b0554502435d3883c528ad0aad1b09b/src/httpserver.cpp#L291-L296

12. in src/httpserver.cpp:244 in 6b58360f9b outdated

    239@@ -239,6 +240,16 @@ static std::string RequestMethodString(HTTPRequest::RequestMethod m)
    240 /** HTTP request callback */
    241 static void http_request_cb(struct evhttp_request* req, void* arg)
    242 {
    243+    // Disable reading to work around a libevent bug, fixed in 2.2.0.
    244+    if (event_get_version_number() < 0x02020001) {
    

    ---

    ---

    

    TheBlueMatt commented at 4:11 pm on November 2, 2017:

    Are they not going to backport the fix? It looks like they haven’t released a 2.2.0?

    ---

    

    theuni commented at 5:56 pm on November 2, 2017:

    Unsure if they’ll backport, but it shouldn’t hurt anything if they do. The reason for the version check is in case the behavior changes drastically in the future and enabling the read/write may be harmful. 2.2.0 isn’t released, but the fix is in master.
13. 
    ryanofsky commented at 5:01 pm on November 2, 2017: member

    I’m trying to reproduce the bug, but not having luck so far. I’m building 6b58360f9b64eb0b680a662fdfd590e47f115f44 with the fix commit reverted. My event_get_version_number() is 0x2001500 and I’m running

    0src/bitcoind -regtest
    

    with

    0while true; do curl -s -XGET --data-binary . 127.0.0.1:18443; done
    

14. in src/httpserver.cpp:615 in 6b58360f9b outdated

    611@@ -601,8 +612,21 @@ void HTTPRequest::WriteReply(int nStatus, const std::string& strReply)
    612     struct evbuffer* evb = evhttp_request_get_output_buffer(req);
    613     assert(evb);
    614     evbuffer_add(evb, strReply.data(), strReply.size());
    615-    HTTPEvent* ev = new HTTPEvent(eventBase, true,
    616-        std::bind(evhttp_send_reply, req, nStatus, (const char*)nullptr, (struct evbuffer *)nullptr));
    617+    auto req_copy = req;
    

    ---

    ---

    

    TheBlueMatt commented at 5:43 pm on November 2, 2017:

    Why do we now need to copy req where we weren’t previously?

    ---

    

    theuni commented at 5:59 pm on November 2, 2017:

    std::bind copied in req by value, but passing “this” into the lambda and using req would use this->req in the callback, which would blow up because req is set to nullptr a few lines below.

    This is clunky, but I’m not sure how else to pass a class member into a lambda by value in c++11.

    ---

    

    TheBlueMatt commented at 6:02 pm on November 2, 2017:

    If the lambda were changed to simply say “req” (not “req&”) then it is copied by value (in this case the pointer is copied).

    ---

    

    theuni commented at 6:39 pm on November 2, 2017:

    Class members need to be captured with ’this’, so unfortunately that doesn’t work.
15. 
    TheBlueMatt commented at 5:44 pm on November 2, 2017: member

    Looks to at least mimic the fix at https://github.com/libevent/libevent/pull/567
16. 
    TheBlueMatt commented at 6:17 pm on November 2, 2017: member

    utACK 6b58360f9b64eb0b680a662fdfd590e47f115f44, assuming https://github.com/libevent/libevent/pull/567 gets merged upstream.
17. rpc: further constrain the libevent workaround

    The bug was introduced in 2.1.6-beta, versions before that don't need the
    workaround.

    97932cd268
18. 
    theuni commented at 6:41 pm on November 2, 2017: member

    Thanks to @ryanofsky for the version info. I bisected the upstream bug and found that it was introduced in 2.1.6-beta, so the workaround has been constrained to the appropriate range.
19. 
    jonasschnelli commented at 7:05 pm on November 2, 2017: contributor

    utACK 97932cd2689659addfbb58dc6148928b73af3bd0 A squash of the two commits would be preferable IMO.
20. 
    laanwj commented at 7:10 pm on November 2, 2017: member

    utACK 97932cd
21. laanwj merged this on Nov 2, 2017
22. laanwj closed this on Nov 2, 2017

    ---

23. laanwj referenced this in commit 7008b07005 on Nov 2, 2017
24. MarcoFalke referenced this in commit 34153a7e4a on Nov 2, 2017
25. MarcoFalke referenced this in commit 8195cb0d7f on Nov 2, 2017
26. MarcoFalke removed the label Needs backport on Nov 2, 2017
27. jasonbcox referenced this in commit c9de86eddb on Mar 9, 2019
28. jonspock referenced this in commit c7a592dd66 on Mar 10, 2019
29. jonspock referenced this in commit 5ff93a3a87 on Mar 11, 2019
30. codablock referenced this in commit 2459b8c758 on Sep 26, 2019
31. codablock referenced this in commit 3e8962323e on Sep 29, 2019
32. barrystyle referenced this in commit f727c04f59 on Jan 22, 2020
33. DeckerSU referenced this in commit c4d87e36fd on Jan 26, 2020
34. DeckerSU referenced this in commit 969471575d on Jan 26, 2020
35. Fair-Exchange referenced this in commit 8e7943fcb3 on May 16, 2020
36. random-zebra referenced this in commit ac523660b4 on Feb 21, 2021
37. LarryRuane referenced this in commit ae7457280b on Mar 3, 2021
38. LarryRuane referenced this in commit b1b9537297 on Mar 3, 2021
39. LarryRuane referenced this in commit 92d8f1d5cc on Apr 13, 2021
40. LarryRuane referenced this in commit af7914242d on Apr 13, 2021
41. zkbot referenced this in commit 0e36226271 on Apr 17, 2021
42. DrahtBot locked this on Sep 8, 2021

Contributors
theuni gmaxwell achow101 dcousens promag TheBlueMatt ryanofsky jonasschnelli laanwj

Labels
RPC/REST/ZMQ

Milestone
0.15.1