Track active requests and wait for last to finish (rpc/rest/zmq)

https://github.com/bitcoin/bitcoin/pull/26742

Host: stickies-v  -  PR author: fjahr

The PR branch HEAD was 6d3a848fc76f76af2b2b262011f37be952325b44 at the time of this review club meeting.

Notes

  • First, we’ll have a more general look at Bitcoin Core’s HTTP server to build an essential understanding of the area we’re working on. Then we’ll dive deeper into the specifics of this PR.

HTTP server

  • Since #5677, Bitcoin Core’s HTTP server is based on libevent2. Libevent is a general purpose event notification library, but is used in Bitcoin Core specifically for HTTP requests (which it supports natively).

  • Much (not all) of the libevent interface is hidden behind wrappers. For example, HTTPRequest wraps evhttp_request and HTTPEvent wraps event_base

  • The relevant workflow for how (for example) an RPC request is handled is roughly as follows:

    1. the HTTP server receives an RPC command from a caller, creates an evhttp_request object and passes its pointer to http_request_cb() (this step is completely handled by libevent)

    2. an HTTPWorkItem is created, containing the evhttp_request (wrapped in HTTPRequest hreq) as well as the path and reference to the handler function (which contains the business logic to be executed to deal with the request)

    3. the HTTPWorkItem is put on the global WorkQueue g_work_queue, which is processed by multiple worker threads asynchronously

    4. when the handler function of a HTTPWorkItem completes successfully, it calls HTTPRequest::WriteReply(), which triggers the libevent function evhttp_send_reply(), which in turn returns a response to the caller and destroys the evhttp_request object.

  • Endpoints are registered to the HTTP server by calling RegisterHTTPHandler(), such as e.g. in StartHTTPRPC()

  • The HTTP server is initiated and started from AppInitServers(), and stopped from Shutdown()

  • StartHTTPServer() adds a thread for each worker to g_thread_http_workers. These threads will keep running until WorkQueue::Interrupt() sets running to false and the queue is empty.

This PR

  • This PR changes StopHTTPServer() to destroy the eventHTTP HTTP server (by calling evhttp_free(eventHTTP)) as soon as all workers have finished executing their tasks, making the shutdown process significantly faster.

  • Libevent requires that no requests are being served when evhttp_free() is called, so we keep track of all the requests created by libevent that have not yet been completed in g_requests.

  • The change is unlikely to be significant to users, since node shutdowns are typically not very frequent. It does however significantly speed up (verify this yourself!) the feature_abortnode.py functional test, consequently speeding up developer workflows worldwide.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. Which of Bitcoin Core’s interface(s) (RPC, REST, ZMQ) make use of the HTTP server? For each interface, how many handlers are registered to pathHandlers?

  3. We already wait for all threads in g_thread_http_workers to be joined before destroying eventHTTP, which only happens when the queue is empty (see notes) - so we would expect all requests to be handled by then. Why do we separately need to track evhttp_request objects, too?

  4. What’s the purpose of g_requests? Can you think of any alternative approaches that would achieve the same goal?

  5. Which (smallest possible) part of the code change in this PR is responsible for the drastic performance speedup in feature_abortnode.py, and why is that making it faster?

  6. What is a GlobalMutex, and why is it used here?

  7. What does the EV_TIMEOUT passed to event_base_once() mean, and why is it used here?

  8. (Bonus): why do we have both a StartRPC() as well as a StartHTTPRPC() function?

Meeting Log

  117:00 <stickies-v> #startmeeting
  217:00 <d33r_gee> hello
  317:00 <effexzi> Hi every1
  417:00 <emzy> hi
  517:00 <svav> Hi there
  617:00 <codo> hi
  717:01 <stickies-v> welcome everyone - thank you for joining us again in the new year! Today we're looking at #26742, authored by fjahr, building on previous work done by promag. The notes and questions are available on https://bitcoincore.reviews/26742
  817:01 <stickies-v> is anyone joining us for the first time today? even if you're just lurking, feel free to say hi!
  917:01 <brunoerg> hi!
 1017:02 <Yaz> hi
 1117:02 <codo> first timer, probably just lurking
 1217:02 <Yaz> first time here, tried to understand as much as possible from the PR :)
 1317:02 <LarryRuane> hi
 1417:03 <rozehnal_paul> hi
 1517:03 <stickies-v> awesome, great to have you here codo, and Yaz ! lurking is great, but feel free to ask or contribute as much as you want
 1617:03 <glozow> hi
 1717:04 <stickies-v> who got the chance to review the PR or read the notes? (y/n)
 1817:04 <rozehnal_paul> y
 1917:04 <Yaz> y
 2017:04 <emzy> n just read notes and pr.
 2117:04 <codo> y
 2217:04 <LarryRuane> n (only a little)
 2317:04 <svav> Read the notes
 2417:05 <stickies-v> it's a pretty small PR in terms of LoC changed, but it pulled me down much deeper a rabbit hole than I expected
 2517:05 <LarryRuane> The condition variable stuff is really cool, but takes some studying to understand
 2617:06 <stickies-v> for those of you who were able to review, would you give it a Concept ACK, Approach ACK, Tested ACK, or NACK?
 2717:06 <stickies-v> LarryRuane: same here! we use it in other places in the code too, but this is the first time I properly looked at it
 2817:06 <LarryRuane> concept ACK for sure
 2917:07 <rozehnal_paul> concept ACK - will expedite testing in the future
 3017:07 <LarryRuane> the concept goes all the way back to the UNIX kernel sleep()/wakeup() synchronization mechanism (which i've always loved)
 3117:07 <LarryRuane> (tells you how old i am!)
 3217:08 <Yaz> I dont know if this should be asked here/know
 3317:08 <Yaz> Why was there a significant performance boost in testing ~31 seconds improvement
 3417:08 <stickies-v> Yaz: we'll cover that in one of the questions - hang tight!
 3517:09 <stickies-v> quick note before diving into the questions: the PR has been updated slightly since these notes were released, so the current commit hashes are different. I'll keep linking to the "old" (6d3a848 and earlier) commits here.
 3617:09 <stickies-v> to start off - how would you briefly summarize what this PR is doing?
 3717:10 <svav> Makes node shutdowns faster so improves developer workflow and productivity.
 3817:12 <stickies-v> svav: yes, that's the purpose of the PR and the main effect of the code
 3917:12 <Yaz> I did not understand the purpose of the PR, unfortunately :(
 4017:13 <rozehnal_paul> Yaz the change is subtle, and not really user-facing. like svav said, nodes can shutdown faster now, a small but noticeable benefit
 4117:13 <stickies-v> most of the code of the PR actually does not (directly) contribute to the performance improvement, but deals with shutting down the HTTP server safely, i.e. ensure we don't kill it before all inbound requests are dealt with in one way or another
 4217:14 <stickies-v> Yaz: an easy way to observe the effect of the PR is by running `./test/functional/feature_abortnode.py` and on this branch. You should notice a drastic speedup on this branch
 4317:15 <stickies-v> alright, moving on to the prep questions
 4417:15 <stickies-v> 1. Which of Bitcoin Core's interface(s) (RPC, REST, ZMQ) make use of the HTTP server? For each interface, how many handlers are registered to `pathHandlers`?
 4517:15 <stickies-v> (as always - feel free to continue discussing previous questions while we move on to the next one)
 4617:15 <svav> Well RPC definitely does ...
 4717:15 <Yaz> Thank you stickies-v rozehnal_paul
 4817:16 <LarryRuane> stickies-v: "You should notice a drastic speedup on this branch" -- you're right, with this branch takes my system about 1.5s, without takes 31s
 4917:17 <rozehnal_paul> 1. definitely rpc interface is involved
 5017:17 <rozehnal_paul> and rest
 5117:17 <LarryRuane> "Which of Bitcoin Core's interface(s) ..." -- I believe all of them do (but I don't know the answer to the second question)
 5217:18 <stickies-v> rozehnal_paul: yes, both RPC and REST rely on the HTTP server! So, ZMQ is the only one that doesn't
 5317:19 <Yaz> I am definitely not on the same level as you are.
 5417:19 <Yaz> Is this related to when a user sends a request to his/her node?
 5517:19 <Yaz> Or is it related to a user relaying blocks/transactions?
 5617:20 <svav> Would I be right in saying ZMQ (ZeroMQ) is lower level and relies on TCP/IP?
 5717:20 <stickies-v> LarryRuane: as a hint, the handlers are stored in `pathHandlers` (https://github.com/bitcoin/bitcoin/blob/329d7e379d09fa5db9c057baa0e100d2b174427d/src/httpserver.cpp#L146)
 5817:21 <stickies-v> Yaz: the RPC, REST and ZMQ interfaces are meant to directly interact with your node (e.g. request or alter information about your wallet, transactions, blocks, ...) in a user-facing way. This is separate from the networking that your node does with other peers, to relay blocks, transactions and addresses etc, which it needs to do in order to be operational
 5917:22 <LarryRuane> stickies-v: seems to be only http and REST
 6017:22 <Yaz> stickies-v (y)
 6117:22 <stickies-v> svav: I'm actually not familiar at all with ZMQ. I just checked whether it was using the HTTP server 🙈 which, it doesn't
 6217:22 <LarryRuane> (i.e. not ZMQ as far as i can tell)
 6317:23 <stickies-v> we have 2 handlers for RPC: https://github.com/bitcoin/bitcoin/blob/9887fc789886b356efc6818783cabaab2b14481f/src/httprpc.cpp#L301-L303
 6417:23 <stickies-v> and another 12 handlers for REST: https://github.com/bitcoin/bitcoin/blob/9887fc789886b356efc6818783cabaab2b14481f/src/rest.cpp#L973-L997
 6517:23 <svav> Is it four handlers per interface?
 6617:24 <LarryRuane> http registers 2 handlers, "/" and "/wallet" ... oh REST registers a bunch of them!
 6717:24 <LarryRuane> (haha sorry i was late with that)
 6817:25 <stickies-v> LarryRuane: yes, which may be surprising given that the RPC interface is much more extensive. For RPC, we've just standardized the way we organize RPC methods quite well, so we can abstract all that away with just 2 handlers. For REST, however, everything is kinda ad-hoc, and every endpoint is its own handler
 6917:26 <LarryRuane> so REST could be improved?
 7017:26 <stickies-v> I just realized my previous answer was not quite nuanced enough
 7117:27 <stickies-v> I think a big reason (speculating) why we have the handlers the way we do, is because REST calls follow the HTTP URI scheme, where the endpoint is encoded in the URI (e.g. http://somewebsite.com/my/endpoint). For RPC, we don't include the method in the URI but in the payload, I think?
 7217:28 <rozehnal_paul> +1 Larrys question
 7317:28 <LarryRuane> if anyone would like to fire up REST, add `-rest` to your config, restart, then for example `curl -s localhost:8332/rest/chaininfo.json|json_pp`
 7417:28 <stickies-v> but the fact remains that for the RPC we have a lot more standardization (`RPCHelpMan`, `RPCArg`, etc...) than for REST
 7517:29 <LarryRuane> (or, you don't really need to `json_pp`, just a pretty-printer)
 7617:29 <stickies-v> 2. We already wait for all threads in `g_thread_http_workers` to be joined before destroying `eventHTTP`, which only happens when the queue is empty (see notes) - so we would expect all requests to be handled by then. Why do we separately need to track `evhttp_request` objects, too?
 7717:31 <svav> Is a pathhandler a Handler that dispatches to a given handler based on a prefix match of the path? Thanks
 7817:32 <stickies-v> svav: yes, exactly, see https://github.com/bitcoin/bitcoin/blob/329d7e379d09fa5db9c057baa0e100d2b174427d/src/httpserver.cpp#L241-L272
 7917:32 <stickies-v> basically the handler tells us which business logic we need to execute in order to satisfy a certain request
 8017:33 <LarryRuane> "Why do we separately need to track `evhttp_request` objects, too?" -- I couldn't figure this out
 8117:33 <stickies-v> LarryRuane: can you think of any synchronization issues?
 8217:34 <LarryRuane> oh, when the queue is empty (and we're not allowing any new requests to be enqueued), there could still be requests in progress?
 8317:34 <rozehnal_paul> for (; i != iend; ++i)
 8417:34 <rozehnal_paul> the first statement that is left empty is shorthand for i starts at 0, correct?
 8517:34 <LarryRuane> rozehnal_paul: no
 8617:35 <stickies-v> rozehnal_paul: we've already initialized `i` two lines higher up
 8717:35 <LarryRuane> also, `i` is not an integer, it's an iterator (often called `it` but here only `i`)
 8817:36 <rozehnal_paul> ah, thx
 8917:36 <stickies-v> LarryRuane: yes, exactly. I'd say in probably >99% of the time they should be synchronized, but if a new request comes in right before stopping the HTTP server, it's possible that the queue is empty and it looks like we can terminate but we actually still need to process that last-minute request
 9017:38 <stickies-v> so, there's a bit of overlap with the next question but it's still worth looking at separately:
 9117:38 <stickies-v> 3. What's the purpose of `g_requests`? Can you think of any alternative approaches that would achieve the same goal?
 9217:38 <LarryRuane> some requests can take a long time, right? like `gettxout`?
 9317:39 <stickies-v> LarryRuane: yes, but that's not really the issue here. Once a request is added onto `g_work_queue`, it will prevent the worker threads from terminating. So we wouldn't terminate the HTTP server prematurely
 9417:40 <svav> Does the g of g_requests stand for global?
 9517:40 <LarryRuane> i know the convention is that the `g_` means it's a global variable (so they're easier to identify when we want to assassinate them later)
 9617:40 <stickies-v> it's just that libevent requires there to be no unhandled requests when we call `evhttp_free(eventHTTP)`, which we need to ensure ourselves
 9717:40 <stickies-v> svav: yes!
 9817:43 <stickies-v> so `g_requests` simply keeps track of all the `evhttp_request` objects created by libevent, so we can easily keep track of whether or not they're all destroyed before we destroy `eventHTTP`
 9917:44 <stickies-v> I see two alternative solutions: we could use a lock for almost the entire duration of `http_request_cb` to ensure that no requests are being processed to be put in the `g_work_queue`, but that would be quite expensive
10017:45 <stickies-v> a more elegant approach is that instead of tracking all the `evhttp_request` objects (or well, pointers to them), we could just keep a simple counter on how many requests we haven't yet handled, as discussed here: https://github.com/bitcoin/bitcoin/pull/26742#discussion_r1063664153
10117:45 <rozehnal_paul> i was curious what the cb stood for in 'http_request_bc'
10217:46 <stickies-v> (however, follow up PRs would benefit from tracking the actual requests instead of a counter)
10317:46 <stickies-v> rozehnal_paul: it stands for callback
10417:46 <stickies-v> https://en.wikipedia.org/wiki/Callback_(computer_programming)
10517:46 <stickies-v> 4. Which (smallest possible) part of the code change in this PR is responsible for the drastic performance speedup in `feature_abortnode.py`, and why is that making it faster?
10617:47 <stickies-v> (Yaz - now we're dealing with your question at the beginning of the review club!)
10717:47 <rozehnal_paul> stickies-v is it the use of the atomic counter?
10817:47 <stickies-v> rozehnal_paul: is that a response to the question of the performance speedup?
10917:48 <Yaz> Thank you for the heads up (y) (y)
11017:49 <rozehnal_paul> yes
11117:49 <stickies-v> then, no. the PR doesn't use an atomic counter, that's just something I suggested but is not implemented (because we need to track the actual requests in a follow up PR, so fjahr would rather not change that again)
11217:51 <rozehnal_paul> now we dont wait for a timeout
11317:51 <rozehnal_paul> https://github.com/bitcoin/bitcoin/pull/26742#issuecomment-1375580720
11417:52 <glozow> https://github.com/bitcoin/bitcoin/pull/26742/files#diff-63c8cb9c9dd61d50d59afd5c39914e1c259f8743030b637a7896a0746c851ef1R493 ?
11517:52 <stickies-v> rozehnal_paul: yes, that's what it boils down to. but why don't we wait for a timeout anymore?
11617:52 <stickies-v> glozow: are you referring to the addition of `event_base_once`?
11717:53 <svav> It's the change to the way StopHTTPServer() works
11817:53 <svav> Now, as soon as all worked have finished executing their tasks, evhttp_free(eventHTTP) is called, destroying the eventHTTP HTTP server
11917:53 <svav> all *workers*
12017:54 <LarryRuane> svav: +1
12117:54 <stickies-v> svav: almost, but not 100%
12217:54 <svav> ok enlighten me
12317:55 <stickies-v> the majority of the code in this PR is to make it go from "as soon as all the workers have finished executing their tasks" to "as soon as all the `evhttp_request` objects created by libevent have been destroyed
12417:55 <stickies-v> in case of the former, this PR could have been limited to just moving `if (g_thread_http.joinable()) g_thread_http.join();` a few lines down, after `evhttp_free(eventHTTP)` has been called (https://github.com/bitcoin-core-review-club/bitcoin/commit/6bd3394c80d2f11ef30c671b03c38985f72df44c#diff-63c8cb9c9dd61d50d59afd5c39914e1c259f8743030b637a7896a0746c851ef1L491)
12517:56 <stickies-v> (you can easily verify this yourself by making just that small change on master, and running `feature_abortnode.py`)
12617:57 <stickies-v> BUT we may encounter the edge case that we call `evhttp_free(eventHTTP)` before all `evhttp_request` objects have been destroyed, which libevent does not allow (I'm not sure if it leads to a crash, UB, or anything else)
12717:57 <LarryRuane> so at line 500 (in the new code), we do call `join()`, is that path not normally taken (only if `eventBase` is not null)?
12817:58 <svav> Re Q6 I would just like to mention that a mutex is analogous to a rubber chicken being used in a meeting which people have to be holding to allow them to talk :')
12917:58 <stickies-v> `eventBase` is never expected to be null, afaict
13017:58 <stickies-v> svav: hahahahahaha please create a PR to update the documentation
13117:59 <stickies-v> does anyone know *why* just switching those few lines leads to such a performance improvement?
13218:00 <svav> Is it something to do with the number of processes that need to be monitored?
13318:00 <stickies-v> svav: I don't think so
13418:01 <codo> My understanding was it closes network connections so the do not linger idle.
13518:01 <LarryRuane> oh wow, so `GlobalMutex` behaves exactly like `Mutex` but this is sort of a documentation thing?
13618:01 <stickies-v> `g_thread_http` is the thread in which we run the HTTP server, and even though all the `evhttp_request`s may be handled, connections to the HTTP server can still be open, albeit idle. Calling `g_thread_http.join()` will not return until also all idle connections are closed
13718:02 <stickies-v> calling `evhttp_free(eventHTTP)` however, will forcefully destroy the HTTP server, cleaning up all idle connections too
13818:02 <stickies-v> so `g_thread_http.join()` now returns immediately
13918:02 <stickies-v> hope that makes sense - we're at time now so I'll close it up here but feel free to keep discussing here, of course
14018:02 <stickies-v> thank you all for joining the conversation!
14118:03 <stickies-v> #endmeeting