Multithreaded JSON-RPC with HTTP 1.1 Keep-Alive support #1101
Conversation
|
Note this is missing some important thread-safety issues I had cleaned up in #568 (which has been upstream-ready for months) |
|
Nothing is -missing-. There is a lock across the entirety of the RPC command execution. |
|
Ah, okay... that's what I missed when comparing the two. |
|
Any limit to the number of threads spawned? Could somebody out-of-memory DoS if they just keep connecting to the RPC port a gazillion times and never closing the connections? |
|
I suspect you'd hit PID limits before memory, and socket limits before that. And since bitcoind uses select(), it would randomly corrupt memory. But that's already a problem anyway... :/ |
|
There is no limit to the threads spawned -- but note that threads are spawned only after checking the IP filter list. The first resource likely to be exhausted is a thread-group or systemwide thread limit, not memory. But yes, if you pass the IP filter list, that can be DoS'd. Should not be a problem to add a simple simultaneous-threads limit here, though. |
|
ACK. I assume the main benefit right now is the keepalive to save constant connection setup/teardown, since RPC calls will still be essentially single-threaded due to obtaining the cs_main and wallet locks. And the secondary benefit is we can eventually move to more fine-grained locks... |
|
Anybody else getting a compiler warning: |
|
Rebased. @gavinandresen do you still get a warning? |
|
Still getting an error: bitcoinrpc.cpp: In function ‘void ThreadRPCServer3(void*)’: Line 2610 is: ... which looks to me like it should be deleted, since loop is defined as: for(;;) in util.h |
|
@gavinandresen fixed. Merge error added some useless code. |
|
Testing this, I got a crash on shutdown of Bitcoin-Qt; see |
| } | ||
| } | ||
| catch (Object& objError) | ||
| { | ||
| ErrorReply(stream, objError, id); | ||
| ErrorReply(conn->stream, objError, id); |
There was a problem hiding this comment.
Shouldn't this be setting fRun to false also? (#1075 removes the first try block entirely, letting this handle errors)
Change internal HTTP JSON-RPC server from single-threaded to thread-per-connection model. The IP filter list is applied prior to starting the thread, which then processes the RPC. A mutex covers the entire RPC operation, because not all RPC operations are thread-safe. [minor modifications by jgarzik, to make change upstream-ready]
|
Rebased, and updated for recent table-driven RPC dispatch. That change simplified our error handling, here. |
Multithreaded JSON-RPC with HTTP 1.1 Keep-Alive support
Multithreaded JSON-RPC with HTTP 1.1 Keep-Alive support
Added cash denomination per BUIP087
This is a cleaned up version of JoelKatz' work, originally found in pull request #568.
A few minor didnt-check-error-return bugs were fixed. All other changes were comment- or coding style-related changes unrelated to behavior.
Should be upstream-ready at this point. Is in use at a few pools and other places.