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

  2. fanquake added the label RPC/REST/ZMQ on Jun 30, 2020
  3. promag commented at 10:51 am on June 30, 2020: member
    Minimal fix in #15363.
  4. 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.

  5. promag force-pushed on Jul 1, 2020
  6. promag marked this as ready for review on Jul 1, 2020
  7. promag commented at 1:54 pm on July 1, 2020: member
    @laanwj ping, again :sweat_smile:
  8. promag marked this as a draft on Jul 3, 2020
  9. 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.
  10. 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.
  11. 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.

  12. fjahr commented at 10:36 pm on May 18, 2021: contributor
    Concept ACK
  13. http: Track active requests and wait for last to finish 662cba4254
  14. http: Relese server before waiting for event base loop exit ea724b9ca4
  15. promag force-pushed on Feb 23, 2022
  16. promag marked this as ready for review on Feb 23, 2022
  17. promag requested review from jonatack on Mar 21, 2022
  18. promag commented at 7:25 pm on March 21, 2022: member
    @laanwj don’t you miss reviewing this code? 😅
  19. 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?

  20. promag commented at 11:27 am on March 23, 2022: member
    @jonatack yes, I think it should be fine, cc @hebasto
  21. 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

     0--- a/src/httpserver.cpp
 1+++ b/src/httpserver.cpp
 2@@ -21,8 +21,10 @@
 3 #include <util/threadnames.h>
 4 #include <util/translation.h>
 5 
 6+#include <condition_variable>
 7 #include <deque>
 8 #include <memory>
 9+#include <mutex>
10 #include <stdio.h>
  22. 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.
  23. 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.

     02022-03-23T11:57:59Z [http] Received a POST request for / from 127.0.0.1:33432
 12022-03-23T11:57:59Z [init] Interrupting HTTP server
 22022-03-23T11:57:59Z [init] tor: Thread interrupt
 32022-03-23T11:57:59Z [init] Shutdown: In progress...
 42022-03-23T11:57:59Z [shutoff] Unregistering HTTP handler for / (exactmatch 1)
 52022-03-23T11:57:59Z [shutoff] Unregistering HTTP handler for /wallet/ (exactmatch 0)
 62022-03-23T11:57:59Z [shutoff] Stopping HTTP server
 72022-03-23T11:57:59Z [shutoff] Waiting for HTTP worker threads to exit
 82022-03-23T11:57:59Z [addcon] addcon thread exit
 92022-03-23T11:57:59Z [torcontrol] RemoveLocal(xxx.onion:38333)
102022-03-23T11:57:59Z [torcontrol] torcontrol thread exit
112022-03-23T11:57:59Z [opencon] opencon thread exit
122022-03-23T11:57:59Z [http] Exited http event loop
132022-03-23T11:57:59Z [shutoff] Waiting for HTTP event thread to exit
142022-03-23T11:57:59Z [shutoff] Stopped HTTP server
152022-03-23T11:57:59Z [net] net thread exit
162022-03-23T11:57:59Z [msghand] msghand thread exit
172022-03-23T11:58:00Z [i2paccept] i2paccept thread exit
182022-03-23T11:58:00Z [shutoff] DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat started
192022-03-23T11:58:00Z [shutoff] DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.01s)
202022-03-23T11:58:00Z [scheduler] scheduler thread exit
212022-03-23T11:58:00Z [shutoff] I2P: Destroying session 99c6a79e84
222022-03-23T11:58:00Z [shutoff] Writing 0 unbroadcast transactions to disk.
232022-03-23T11:58:00Z [shutoff] Dumped mempool: 7.4e-05s to copy, 0.008535s to dump
242022-03-23T11:58:00Z [shutoff] [default wallet] Releasing wallet
252022-03-23T11:58:00Z [shutoff] [blank] Releasing wallet
262022-03-23T11:58:00Z [shutoff] [encrypted blank descriptor] Releasing wallet
272022-03-23T11:58:00Z [shutoff] [disable private keys] Releasing wallet
282022-03-23T11:58:00Z [shutoff] [disable private keys and blank] Releasing wallet
292022-03-23T11:58:00Z [shutoff] [default settings] Releasing wallet
302022-03-23T11:58:00Z [shutoff] Shutdown: done

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

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

  25. fanquake referenced this in commit d906329c28 on Apr 6, 2022
  26. achow101 commented at 6:10 pm on October 12, 2022: member
    Are you still working on this?
  27. 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.
  28. achow101 closed this on Nov 10, 2022
  29. fjahr commented at 6:50 pm on December 22, 2022: contributor
    I have reopened this PR in #26742. Comments by @jonatack have been addressed.
  30. promag deleted the branch on Dec 22, 2022
  31. achow101 referenced this in commit 86bacd75e7 on Mar 7, 2023
  32. sidhujag referenced this in commit 61edeffbd5 on Mar 7, 2023
  33. fanquake referenced this in commit db7b5dfcc5 on Oct 4, 2023
  34. Frank-GER referenced this in commit b0fea09705 on Oct 13, 2023
  35. bitcoin locked this on Dec 22, 2023


promag DrahtBot jonatack fjahr hebasto achow101

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me