cli: Replace libevent usage with simple http client#34342
cli: Replace libevent usage with simple http client#34342fjahr wants to merge 3 commits intobitcoin:masterfrom
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/34342. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste 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. |
|
🚧 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. |
eb70933 to
7a27975
Compare
7a27975 to
1109a00
Compare
This will be shared between the http server and the bitcoin-cli http client code.
1109a00 to
efd74a8
Compare
|
concept ACK |
bradleystachurski
left a comment
There was a problem hiding this comment.
Concept ACK (and supportive of the broader libevent removal effort)
Reviewed efd74a8
Found a minor behavioral regression: connection failures (e.g. unresolvable host, non-listening port) lose the help text shown on master.
Master:
error: timeout on transient error: Could not connect to the server 127.0.0.1:19999
Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
Use "bitcoin-cli -help" for more info.
Branch:
error: timeout on transient error: Could not connect to the server 127.0.0.1:19999
Catching instead of re-throwing preserves the help text:
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 8414ad0317..3cd6e4b29c 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -1258,7 +1258,7 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
try {
response = client.Post(endpoint, headers, strRequest);
} catch (const CConnectionFailed&) {
- throw;
+ response.status = 0;
}
if (response.status == 0) {Verified this preserves the help text and passes interface_bitcoin_cli.py.
Replace libevent-based HTTP client with a simple synchronous implementation using the Sock class directly.
efd74a8 to
58d7fe0
Compare
You were right, good catch! Your suggested fix looks good to me as well, I have just applied that change. |
| std::string Stringify() const; | ||
|
|
||
| private: | ||
| std::unordered_map<std::string, std::string, util::AsciiCaseInsensitiveHash, util::AsciiCaseInsensitiveKeyEqual> m_map; |
There was a problem hiding this comment.
I think there's a few things we learned in #34158 as well that can be applied. This PR (bitcoin-cli) can maybe be draft until torcontol is ironed out.
There was a problem hiding this comment.
It is already draft but thanks for the hints, I will address these once we have made some more progress on torcontol and the server.
|
Concept ACK |
Part of the effort to remove the libevent dependency altogether, see #31194
This takes the
HTTPHeadersclass from #32061 and puts it intocommon/http.h|cpp. While it would not be strictly required for that PR to go in first, it might be better to review it first or simultaneously.Otherwise the change itself replaces the libevent-based HTTP client with a simple synchronous implementation which uses the
Sockclass directly.