-
Notifications
You must be signed in to change notification settings - Fork 36.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libevent-based http server #5677
Conversation
df020f5
to
bed7a45
Compare
Very nice! Before looking over the work itself, I wanted to be sure that libevent was viable for all of our build targets. See here for the build-system work. This should be enough to get Travis passing, I'd think: |
@cfields Will pull that in, thanks a lot! |
Hm, nothing special needed for longpolling? |
@luke-jr I don't think so. The current implementation should work. Of course it would be more optimal to release the worker thread while longpolling, and change "new block" it into an event-trigger, but I leave that as a challenge for later. |
c527672
to
8d45fe4
Compare
The fail in "32-bit + dash" is strange
I'm not sure how this can be affected at all (passes fine here), but I'll check. On Win32/64 it still tries to link against libevent_pthread. IIRC there is no specific thread library for windows,
|
Blah, sorry, missed that one. |
It's easy to miss those sneaky qt unit tests. Windows passes now! That leaves the
... no clue how this happens yet. The Qt tests don't use the RPC mechanism. My gut feeling is some interaction with OpenSSL which, absent verification, is now an indirect dependency through Qt? Not sure, and why it only happens in this test is a mystery to me. |
@laanwj Yes, it has some interaction with qt:
|
tested gitian build. |
4054c36
to
4281bb8
Compare
Concept ACK |
c532cb5
to
6f1259a
Compare
Now that the code is stable it is time for some benchmarking. GET request to invalid URLThese are handled by evhttp itself, so this is the baseline.
As expected, we get 404s. GET request to /These are dispatched to a worker thread. Some more latency is expected. In the worker thread these error out early, as GET is not a valid method for JSON RPC.
As expected: lots of 405 (invalid method for URL) results. RPC getgenerate requestsPost wrk.method = "POST"
wrk.body = "{\"method\":\"getgenerate\",\"params\":[],\"id\":1}\n"
wrk.headers["Content-Type"] = "application/json"
wrk.headers["Authorization"] = "Basic XXX"
All responses succesful. RPC getinfo requestsPost wrk.method = "POST"
wrk.body = "{\"method\":\"getinfo\",\"params\":[],\"id\":1}\n"
wrk.headers["Content-Type"] = "application/json"
wrk.headers["Authorization"] = "Basic XXX
Although the performance goes down, no errors happen. The maximum queue depth is never reached. RPC requests w/ invalid authenticationPost wrk.method = "POST"
wrk.body = "{\"method\":\"getinfo\",\"params\":[],\"id\":1}\n"
wrk.headers["Content-Type"] = "application/json"
wrk.headers["Authorization"] = "Basic YYY
Increasing the load further, we can exceed the work queue depth:
Looks good. Nothing unexpected, it's clear the evhttp is not the bottleneck, and the work queue works as expected. Will try this with the old asio-based HTTP server shortly. |
Old http serverSame steps as above, repeated with the old server as of commit 944c256. GET request to invalid URL
GET request to /
RPC getgenerate requests
RPC getinfo requests
RPC requests w/ invalid authentication
Summary
Take into account that it's not entirely a fair comparison: the new http server can service a large number of connections at the same time, whereas the old server can have a maximum of four (or, |
Why are we dropping SSL support for RPC? |
|
||
// Synchronously look up hostname | ||
struct evhttp_connection *evcon = evhttp_connection_base_new(base, NULL, host.c_str(), port); // XXX RAII | ||
if (evcon == NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to understand why are you explicitly using == NULL here and for base above just !var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Diapolo == NULL
and !evcon
are equivalent in this case so purely a matter to taste. The pull clearly states [PoC] as for proof-of-concept, please don't report all these minor things but only critical or high-level issues,
@Diapolo Re: this pull: because I don't feel like implementing it. In the longer term I (and many others) would also argue it is better to drop it:
|
I agree with @laanwj. SSL support could lead somebody to believe it's "save". IMO it currently a bad idea to expose bitcoind RPC to a public accessible area. Nevertheless, If one like to do this, he could still do a apache ssl enable reverse proxy to bitcoind's RPC. |
Also SSL in the RPC massively increases the attack surface we have exposed (if you also expose it to the outside world) and we've had to push updates previously on account of it-- even though we believe its a feature virtually no one uses. As mentioned it can be better accomplished via stunnel (or any of several other tools) |
I agree, it was a mistake to add SSL support to the RPC (mea culpa-- I wrote the original version of that code). |
concept ACK. |
@paveljanik: just added |
post merge tested re-ACK |
Github-Pull: bitcoin#5677 Rebased-From: 26c9b83
Github-Pull: bitcoin#5677 Rebased-From: 26c9b83
libevent-based http server Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#5677 - bitcoin/bitcoin#6695 - bitcoin/bitcoin#6899 - bitcoin/bitcoin#7016 - bitcoin/bitcoin#7964 - bitcoin/bitcoin#8722 - bitcoin/bitcoin#8730 - bitcoin/bitcoin#9073 - bitcoin/bitcoin#9265 - bitcoin/bitcoin#9387 - bitcoin/bitcoin#9471 - bitcoin/bitcoin#9647 - bitcoin/bitcoin#9903 Closes #1593 and #1856.
libevent-based http server Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#5677 - bitcoin/bitcoin#6695 - bitcoin/bitcoin#6899 - bitcoin/bitcoin#7016 - bitcoin/bitcoin#7964 - bitcoin/bitcoin#8722 - bitcoin/bitcoin#8730 - bitcoin/bitcoin#9073 - bitcoin/bitcoin#9265 - bitcoin/bitcoin#9387 - bitcoin/bitcoin#9471 - bitcoin/bitcoin#9647 - bitcoin/bitcoin#9903 - bitcoin/bitcoin#6640 - bitcoin/bitcoin#8139 - bitcoin/bitcoin#8839 Closes #1593 and #1856.
By using a proven, high-performance asynchronous networking library (also used by Tor) and HTTP server, problems such as #5674, #5655, #344 should be avoided.
What works? bitcoind, bitcoin-cli, bitcoin-qt. Unit tests and RPC/REST tests pass. The aim for now is everything but SSL support.
Configuration options:
-rpcthreads
: repurposed as "number of work handler threads". Still defaults to 4.-rpcworkqueue
: maximum depth of work queue. When this is reached, new requests will return a 500 Internal Error.-rpctimeout
: inactivity time, in seconds, after which to disconnect a client.-debug=http
: low-level http activity logging(due to the separation of RPC and HTTP server, renaming these options may make sense, but I've kept this out of backwards compatiblity)
TODO:
Build system (currently hardcodes libraries, so this will definitely not pass Travis)(thanks @theuni)REST and RPC register their own request handlers respectivelyQt debug console must register a RPCTimerInterface (to make timeouts in the debug console work with -server=0)Interrupt/Shutdown flow needs to be cleaned up[warn] event_active: event has no event_base set.
appears sometimes to the console. Seems to be harmless, but it is weird (see @ajweiss comments)