This patch set updates bitcoin’s RPC server to use boost::asio’s async_read, _write, etc instead of their non-asynchronous versions.
The server still exists as a single separate thread, but the async calls should greatly improve performance under high load. The next step in robustifying the RPC server is to create a pool of threads for the RPC server, each of which handle a number of asynchronously reading and writing connections.
Properly does ASIO for both SSL and non-SSL cases if SSL is enabled, and
also if SSL is disabled.
Next steps: attempt to non-block on the actual RPC call.
make the whole thing a thread poolUsing this patch on a heavily loaded bitcoind, I am getting quite a few: reservekeyfromkeypool(): read failed from send transactions.
I backed out the patch and so far so good but time will tell. Are you aware of any wallet locking issues with this async code?
I don’t have the dumps I made handy but I’ve seen this issue as well. It only seems to happen on large RPC calls like listtransactions. I actually ran across it testing out sipa’s showwallet branch for privkey import/export and didn’t make the correlation until seeing your comments.
Reverting this makes it stop happening. So there’s definitely an issue here and this shouldn’t be pulled until resolved.
The dumps I did showed it just cutting off in the middle sometimes or sometimes leaving out chunks of the response that made the json not validate.
2037+ shared_from_this(), asio::placeholders::error,
2038+ asio::placeholders::bytes_transferred));
2039+ }
2040+ void respond(std::string str)
2041+ {
2042+ boost::asio::async_write(socket, boost::asio::buffer(str),
See comment above. A possible fix: http://pastie.org/private/d04wwgdsk881yl70k1g
I’m not familiar with boost though, so there might very well be a better solution.
If your plan is to optimize the RPC code, you’re kind of barking up the wrong tree. The socket I/O is only about 3% of the load coming from the RPC code. If you just want it to handle more than one connection at a time and keepalives, just dispatching the RPC to a thread (and fixing the protocol handler to close the socket when it’s the right time) does it.
The breakdown is, roughly: 30% Do the actual operation (various based on the operation, of course) 20% Parse JSON request 20% Generate JSON reply 15% Parse HTTP 4% Check user/pass/IP 3% Socket I/O 2% Make JSON reply 1% Send HTTP reply
This is still the right thing to do. However, there are much simpler and less invasive ways to accomplish the same thing. Now, if you had done this for the net code, that would actually make a huge difference.
@JoelKatz: you missed the point. This is not an optimization, but a big step towards correcting a major design flaw. The current RPC code executes HTTP requests in order, in a FIFO queue.
You can find examples of this logic in “My First TCP Server” style code examples, but never in any production server. A synchronous, FIFO approach stalls all clients except the “current” one. If the current client is, itself, stalled or slow or misbehaving, then all other clients suffer.
Asynchronous I/O + HTTP/1.1 keep alives are desperately needed to solve obvious problems seen in the field by heavy RPC users.
I don’t think many people, if any, expose their bitcoin RPC connections to the world. So I don’t think the DoS issue is particularly important. I guess you could argue that they don’t because they can’t, and if they could, they might. But I think most people run a bitcoin client on each machine that needs to issue RPC queries and should bind their RPC listening sockets to ’localhost’ only.
As for RPC not being scalable in modern workloads, I would argue that modern workloads include proper support for keepalives and don’t require large numbers of connections.
If there’s a realistic case where asio for RPC provides any benefit over thread-per-connection with proper keepalive support, I don’t know what it is. Again, I still think this patch is “the right thing to do”, but I don’t think there will be any actual benefit in any realistic use case. (Again, over dipatching the RPC calls to threads and fixing the broken HTTP keepalive implementation. You have to at least do that.)
The pull has become unmergeable (without conflicts), and will be closed in 15 days from this message if action is not taken.
To prevent closure, kindly rebase the pull to merge cleanly with the current codebase. If a time extension is needed, please respond to this comment or contact QA@BitcoinTesting.org.