Replace libevent with our own HTTP and socket-handling implementation #32061
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32061. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_4_m |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Concept ACK, nice work |
|
Concept ACK |
|
Concept ACK My understanding from the low-level networking discussion at CoreDev was that this wouldn't build on top of sockman. I guess the devil is in the details but can you address that in what sense the current approach follows what was discussed there? Thanks! |
Sure, by coredev I had already written most of this implementation (based on sockman) but the performance was bad, and that was part of the motivation behind the deep-dive talk. However, by the end of the week I had reviewed that code in person with smart attendees and not only improved the performance of my code but started to improve performance vs master branch as well! Those updates came in the days just after the deep-dive discussion. SOME kind of sockman is needed to replace libevent. The one @vasild wrote does actually seem to work well for this purpose as well as for p2p, and it would be "nice" to only have to maintain one I/O loop structure in bitcoind. @theuni is investigating how a sockman for http could be optimized if it had no other purpose, and I think that is the kind of feedback that will help us decide which path to take. |
640f38f to
49784b3
Compare
💯 |
See https://www.rfc-editor.org/rfc/rfc7230#section-6.3.2 > A server MAY process a sequence of pipelined requests in parallel if they all have safe methods (Section 4.2.1 of [RFC7231]), but it MUST send the corresponding responses in the same order that the requests were received. We choose NOT to process requests in parallel. They are executed in the order recevied as well as responded to in the order received. This prevents race conditions where old state may get sent in response to requests that are very quick to process but were requested later on in the queue.
This is a refactor to prepare for matching the API of HTTPRequest definitions in both namespaces http_bitcoin and http_libevent. In particular, to provide a consistent return type for GetRequestMethod() in both classes.
These methods are called by http_request_cb() and are present in the original http_libevent::HTTPRequest.
The original function is passed to libevent as a callback when HTTP requests are received and processed. It wrapped the libevent request object in a http_libevent::HTTPRequest and then handed that off to bitcoin for basic checks and finally dispatch to worker threads. In this commit we split the function after the http_libevent::HTTPRequest is created, and pass that object to a new function that maintains the logic of checking and dispatching. This will be the merge point for http_libevent and http_bitcoin, where HTTPRequest objects from either namespace have the same downstream lifecycle.
The original function was already naturally split into two chunks: First, we parse and validate the users' RPC configuration for IPs and ports. Next we bind libevent's http server to the appropriate endpoints. This commit splits these chunks into two separate functions, leaving the argument parsing in the common space of the module and moving the libevent-specific binding into the http_libevent namespace. A future commit will implement http_bitcoin::HTTPBindAddresses to bind the validate list of endpoints by the new HTTP server.
aa89c97 to
cdb13c9
Compare
This removes the dependency on libevent for scheduled events, like re-locking a wallet some time after decryption.
cdb13c9 to
e531a7c
Compare
test/functional/interface_http.py
Outdated
| conn = http.client.HTTPConnection(urlNode2.hostname, urlNode2.port) | ||
| conn.connect() | ||
| sock = conn.sock | ||
| sock.settimeout(1) |
test/functional/interface_http.py
Outdated
| # The server should not respond to the fast, second request | ||
| # until the (very) slow first request has been handled: | ||
| res = sock.recv(1024) | ||
| assert not res |
test/functional/interface_http.py
Outdated
| body_chunked = [ | ||
| b'{"method": "submitblock", "params": ["', | ||
| b'0A' * 1000000, | ||
| b'0B' * 1000000, | ||
| b'0C' * 1000000, | ||
| b'0D' * 1000000, | ||
| b'"]}' | ||
| ] |
test/functional/interface_http.py
Outdated
| headers=headers_chunked, | ||
| encode_chunked=True) | ||
| out1 = conn.getresponse().read() | ||
| assert out1 == b'{"result":"high-hash","error":null}\n' |
test/functional/interface_http.py
Outdated
| # definitely closed | ||
| try: | ||
| conn.request('GET', '/') | ||
| conn.getresponse() | ||
| # macos/linux windows | ||
| except (ConnectionResetError, ConnectionAbortedError): | ||
| pass |
src/httpserver.cpp
Outdated
| void HTTPServer::CloseConnectionInternal(std::shared_ptr<HTTPClient>& client) | ||
| { | ||
| event_free(ev); | ||
| if (CloseConnection(client->m_node_id)) { | ||
| LogDebug(BCLog::HTTP, "Disconnected HTTP client %s (id=%d)\n", client->m_origin, client->m_node_id); | ||
| } else { | ||
| LogDebug(BCLog::HTTP, "Failed to disconnect non-existent HTTP client %s (id=%d)\n", client->m_origin, client->m_node_id); | ||
| } | ||
| } |
src/httpserver.h
Outdated
| // Set to true when we receive request data and set to false once m_send_buffer is cleared. | ||
| // Checked during DisconnectClients(). All of these operations take place in the Sockman I/O loop, | ||
| // however it may get set my a worker thread during an "optimistic send". | ||
| std::atomic_bool m_prevent_disconnect{false}; |
src/httpserver.h
Outdated
| std::string Stringify() const; | ||
|
|
||
| private: | ||
| std::map<std::string, std::string, util::CaseInsensitiveComparator> m_map; |
There was a problem hiding this comment.
This is great feedback thanks. I made this change by first editing the "string: add CaseInsensitiveComparator" commit which is now "string: add CaseInsensitive{KeyEqual, Hash} for unordered map" and implements the two operators needed for the unordered map.
src/httpserver.cpp
Outdated
| std::optional<std::string_view> HTTPHeaders::Find(const std::string key) const | ||
| { | ||
| if (!InitHTTPAllowList()) | ||
| return false; | ||
| const auto it = m_map.find(key); | ||
| if (it == m_map.end()) return std::nullopt; | ||
| return std::string_view(it->second); |
There was a problem hiding this comment.
Ah that is another great catch thank you. Glad I wrote that as an extra commit! I'll pop it off, it was added in response to this review comment: #32061 (comment)
src/httpserver.cpp
Outdated
| auto content_length_value{m_headers.Find("Content-Length")}; | ||
| if (!content_length_value) return true; | ||
|
|
||
| uint64_t content_length; | ||
| if (!ParseUInt64(content_length_value.value(), &content_length)) throw std::runtime_error("Cannot parse Content-Length value"); | ||
|
|
||
| // Not enough data in buffer for expected body | ||
| if (reader.Left() < content_length) return false; | ||
|
|
||
| m_body = reader.ReadLength(content_length); |
There was a problem hiding this comment.
Yeah its true, a client could send a request with a huge list of HTTP headers and an incomplete body and we would parse the headers over and over again from m_recv_buffer until the body was finally completed, when we finally pinch off a HTTPRequest and erase the data from the buffer.
There certainly is an optimization there, keeping an in-progress request attached to HTTPClient with a "where we left off" pointer. It sounds like you would be ok with seeing that in a follow-up PR but let me know if its blocking for you here.
|
Low-latency HTTP request benchmark improvement - compared with #32541 (comment) taking ~130ms per request: |
|
The latency of 404 response has improved too:
|
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
|
||
| /** Change logging level for libevent. */ | ||
| void UpdateHTTPServerLogging(bool enable); | ||
| namespace http_bitcoin { |
There was a problem hiding this comment.
Just an idea, and not necessarily a suggestion for this PR since it could be a followup, but I was thinking since httpserver.h and httpserver.cpp files are basically rewritten here it could also be good to rename them to rpc/http/server.h and rpc/http/server.cpp and put them in a matching rpc::http namespace. Reasons for suggesting this:
- To take some files out of the top level directory.
- To choose a namespace name
rpc::httpthat matches the directory pathrpc/http. - To make space for a client library to be located at
rpc/http/client.hrpc/http/client.cpp(for tools that may that want to use somebitcoin-clifunctionality without shelling out tobitcoin-cli) - To make space for other RPC protocols to be implemented places like
rpc/grpc/. - To make
rpc/andipc/layouts more consistent (since latter usessrc/ipc/<protocol>structure).
One reason to not like this suggestion would be if you wouldn't consider other uses of the HTTP server to be RPC. But I would interpret "rpc" liberally and probably move the related files as well:
rest.h->rpc/http/rest.hrest.cpp->rpc/http/rest.cpphttprpc.h->rpc/http/jsonrpc.hhttprpc.cpp->rpc/http/jsonrpc.cpp
Again not pushing for this, just wanted to mention the idea.
This is a major component of removing libevent as a dependency of the project
It is based on #30988 but only in the sense that it copies the
Sockmanclass introduced in that PR. The p2p /Connmanrefactor isn't needed for HTTP and therefore this branch can be reviewed and merged independently of the p2p changes.Commit strategy:
sockman.{h,cpp}from Split CConnman #30988http_libeventhttp_bitcoin(the namespace manages duplicateHTTPRequestdefinitions, etc)http_libevent)I am currently seeing about a 10% speed up in the functional tests on my own arm/macos machine.
Integration testing:
I am testing the new HTTP server by forking projects that integrate with bitcoin via HTTP and running their integration tests with bitcoind built from this branch (on Github actions). I will continue adding integrations over time, and re-running these CI tests as this branch gets rebased:
rpc-bitcoin