http: Track active requests and wait for last to finish #19420

pull promag wants to merge 2 commits into bitcoin:master from promag:2020-06-track-requests changing 1 files +28 −4

promag commented at 10:38 AM on June 30, 2020: member

This PR calls evhttp_free before releasing event base. But according to evhttp_free docs:

Works only if no requests are currently being served.

So this PR also tracks active requests with libevent and waits for last request to finish. This requires libevent 2.1 due to evhttp_request_set_on_complete_cb (https://github.com/libevent/libevent/blob/master/whatsnew-2.1.txt).

Finally, the call to evhttp_free is done in the event base loop to avoid concurrency issues.

Now test/functional/feature_abortnode.py quits normally, not due to socket timeouts.
fanquake added the label RPC/REST/ZMQ on Jun 30, 2020
promag commented at 10:51 AM on June 30, 2020: member

~~Minimal fix in #15363.~~
DrahtBot commented at 12:59 PM on June 30, 2020: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
promag force-pushed on Jul 1, 2020
promag marked this as ready for review on Jul 1, 2020
promag commented at 1:54 PM on July 1, 2020: member

@laanwj ping, again :sweat_smile:
promag marked this as a draft on Jul 3, 2020
promag commented at 1:34 AM on July 3, 2020: member

There is an issue which is fixed in #19434 - complete callback isn't called if client closes the connection.
jonatack commented at 10:24 AM on July 5, 2020: contributor

Interesting! -- this PR brings the run time of test/functional/feature_abortnode.py for me, with a debug build, from 32 seconds down to only 2.
jonatack commented at 3:27 PM on July 5, 2020: contributor

Concept ACK, the code looks fine, tests pass and feature_abortnode.py for instance runs much faster.

Also, you may want to see https://github.com/libevent/libevent/issues/643 and https://github.com/libevent/libevent/pull/591.
fjahr commented at 10:36 PM on May 18, 2021: contributor

Concept ACK
http: Track active requests and wait for last to finish 662cba4254
http: Relese server before waiting for event base loop exit ea724b9ca4
promag force-pushed on Feb 23, 2022
promag marked this as ready for review on Feb 23, 2022
promag requested review from jonatack on Mar 21, 2022
promag commented at 7:25 PM on March 21, 2022: member

@laanwj don't you miss reviewing this code? 😅
jonatack commented at 11:25 AM on March 23, 2022: contributor

So this PR also tracks active requests with libevent and waits for last request to finish. This requires libevent 2.1 due to evhttp_request_set_on_complete_cb (libevent/libevent@master/whatsnew-2.1.txt).

Does this change require bumping the minimum enforced libevent version from 2.0.21 to 2.1?
promag commented at 11:27 AM on March 23, 2022: member

@jonatack yes, I think it should be fine, cc @hebasto

in src/httpserver.cpp:151 in ea724b9ca4

 145 | @@ -146,6 +146,10 @@ static std::unique_ptr<WorkQueue<HTTPClosure>> g_work_queue{nullptr};
 146 |  static std::vector<HTTPPathHandler> pathHandlers;
 147 |  //! Bound listening sockets
 148 |  static std::vector<evhttp_bound_socket *> boundSockets;
 149 | +//! Track active requests
 150 | +static Mutex g_requests_mutex;
 151 | +static std::condition_variable g_requests_cv;

jonatack commented at 11:39 AM on March 23, 2022:

662cba4254b8f54d02f7d64717ae5a7e13b5a502 maybe add these headers

--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -21,8 +21,10 @@
 #include <util/threadnames.h>
 #include <util/translation.h>
 
+#include <condition_variable>
 #include <deque>
 #include <memory>
+#include <mutex>
 #include <stdio.h>

in src/httpserver.cpp:485 in ea724b9ca4
```
 480 | +        while (!g_requests.empty()) {
 481 | +            g_requests_cv.wait(lock);
 482 | +        }
 483 | +    }
 484 | +    if (eventHTTP) {
 485 | +        event_base_once(eventBase, -1, EV_TIMEOUT, [](evutil_socket_t, short, void*) {
```
jonatack commented at 11:50 AM on March 23, 2022:

ea724b9 curious why you added this line (maybe describe the change in the commit message)

The other place in this codebase that calls event_base_once() to schedule a one-time event is torcontrol.cpp in InterruptTorControl.

promag commented at 4:29 PM on March 23, 2022:

This is a way to call evhttp_free in the event base thread, meaning it's not called concurrently with the handling of unfinished request connections.

jonatack commented at 11:56 AM on March 23, 2022: contributor

Light approach ACK ea724b9ca40c58881ad51058079641c1e5edb81b based on code review, looked at the libevent repo, doc/dependencies and configure.ac, rebased, debug build, ran unit tests, re-verified that test/functional/feature_abortnode.py runs far faster (under 2 seconds with debug build on a slow CPU), ran mainnet/signet/testnet nodes with http logging on and made a few CLI calls, checked nodes shutdown. Modulo we would need to bump the minimum required version of libevent from 2.0.21 to 2.1.

<details><summary>debug log on signet shutdown</summary><p>

2022-03-23T11:57:59Z [http] Received a POST request for / from 127.0.0.1:33432
2022-03-23T11:57:59Z [init] Interrupting HTTP server
2022-03-23T11:57:59Z [init] tor: Thread interrupt
2022-03-23T11:57:59Z [init] Shutdown: In progress...
2022-03-23T11:57:59Z [shutoff] Unregistering HTTP handler for / (exactmatch 1)
2022-03-23T11:57:59Z [shutoff] Unregistering HTTP handler for /wallet/ (exactmatch 0)
2022-03-23T11:57:59Z [shutoff] Stopping HTTP server
2022-03-23T11:57:59Z [shutoff] Waiting for HTTP worker threads to exit
2022-03-23T11:57:59Z [addcon] addcon thread exit
2022-03-23T11:57:59Z [torcontrol] RemoveLocal(xxx.onion:38333)
2022-03-23T11:57:59Z [torcontrol] torcontrol thread exit
2022-03-23T11:57:59Z [opencon] opencon thread exit
2022-03-23T11:57:59Z [http] Exited http event loop
2022-03-23T11:57:59Z [shutoff] Waiting for HTTP event thread to exit
2022-03-23T11:57:59Z [shutoff] Stopped HTTP server
2022-03-23T11:57:59Z [net] net thread exit
2022-03-23T11:57:59Z [msghand] msghand thread exit
2022-03-23T11:58:00Z [i2paccept] i2paccept thread exit
2022-03-23T11:58:00Z [shutoff] DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat started
2022-03-23T11:58:00Z [shutoff] DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.01s)
2022-03-23T11:58:00Z [scheduler] scheduler thread exit
2022-03-23T11:58:00Z [shutoff] I2P: Destroying session 99c6a79e84
2022-03-23T11:58:00Z [shutoff] Writing 0 unbroadcast transactions to disk.
2022-03-23T11:58:00Z [shutoff] Dumped mempool: 7.4e-05s to copy, 0.008535s to dump
2022-03-23T11:58:00Z [shutoff] [default wallet] Releasing wallet
2022-03-23T11:58:00Z [shutoff] [blank] Releasing wallet
2022-03-23T11:58:00Z [shutoff] [encrypted blank descriptor] Releasing wallet
2022-03-23T11:58:00Z [shutoff] [disable private keys] Releasing wallet
2022-03-23T11:58:00Z [shutoff] [disable private keys and blank] Releasing wallet
2022-03-23T11:58:00Z [shutoff] [default settings] Releasing wallet
2022-03-23T11:58:00Z [shutoff] Shutdown: done

</p></details>

Commit message: "http: Relese server before waiting for event base loop exit" -> s/Relese/Release/

hebasto commented at 8:03 AM on March 26, 2022: member

Does this change require bumping the minimum enforced libevent version from 2.0.21 to 2.1?

Done in #24681.
fanquake referenced this in commit d906329c28 on Apr 6, 2022
achow101 commented at 6:10 PM on October 12, 2022: member

Are you still working on this?
achow101 commented at 5:12 PM on November 10, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
achow101 closed this on Nov 10, 2022
fjahr commented at 6:50 PM on December 22, 2022: contributor

I have reopened this PR in #26742. Comments by @jonatack have been addressed.
promag deleted the branch on Dec 22, 2022
achow101 referenced this in commit 86bacd75e7 on Mar 7, 2023
sidhujag referenced this in commit 61edeffbd5 on Mar 7, 2023
fanquake referenced this in commit db7b5dfcc5 on Oct 4, 2023
Frank-GER referenced this in commit b0fea09705 on Oct 13, 2023
bitcoin locked this on Dec 22, 2023

Contributors

Labels

Linked (view graph)

#19434 http: Detect remote disconnect #26742 http: Track active requests and wait for last to finish - 2nd attempt #28551 http: bugfix: allow server shutdown in case of remote client disconnection

http: Track active requests and wait for last to finish #19420

Conflicts