The commits in this pull request modify the RPC-connection handling code in such a way that listening for and accepting of incoming connections is performed asynchronous (reading/writing is still synchronous).
This allows for listening on multiple sockets at once, which I use in one of the other commits to implement dual IPv4/IPv6 support.
It currently still is single-threaded, i.e. all code is still executed from ThreadRPCServer2 via the “io_service.run_one()” construct.
The way it basically works is that certain actions are started (using ‘async_*’ methods), and get passed along with them an event handler to be called upon that action’s completion. The io_service object manages these actions, waits for any of them to complete then dispatches the appropriate event handler.
All of that happens only within the threads from which you call io_service.run(), io_service.run_one(), io_service.poll() or io_service.poll_one(). As a result this allows future expansion into multiple threads by simple invoking io_service.run() in a new thread, but doesn’t inherently add more threads (just allows for it).
Well, all that can be supported about IPv6 for RPC is in this branch. So that’s exactly what this branch is: a “support IPv6 for RPC” branch.
Given that the RPC code is completely separate from any other networking code it actually makes sense to migrate it separately. That’s why I’m not even trying to support IPv6 across all of bitcoin at once, incremental changes tend to work better in my experience.
Adding IPv6 (or any other protocol that requires an additional listening socket) requires event-driven (aka asynchronous I/O). That or a separate thread per listening socket, wich conflicts with the RPC’s assumption that RPC code is single threaded…
Additionally event-driven approaches tend to scale better (less context switching, locking and per-thread resources overhead).
Another (probably better) solution would be to have a select(2)-like event-based processing loop. It would have the single-thread advantage of asynchronous I/O but the simplicity of a callback-less design. As I assume that the addition of callbacks in my current implementation is what you like least? (Please confirm/deny that last question/statement.)
That should localise most of the changes to the place where current code calls listener.accept(socket). This solution I should be able to implement rather quickly in the weekend.
Yes, but that is unrelated to my patch.
The alternate implementation of IPv6 support I’m thinking of would look somewhat like this pseudocode:
the mainloop would then be this:
That approach would still have a single callback, but only to accept tue connection, not to handle it. If there are no objections to this approach I’ll implement it this weekend.
The reason it conflicts with it is simple of course: there was no multithreaded RPC patch when I wrote this patch.
As for resolving those conflicts by implementing on top of #568, no promises but I’ll look at it in the weekend. Right now I’m going to get some much needed sleep.
2524+ && (address.to_v6().is_v4_compatible()
2525+ || address.to_v6().is_v4_mapped()))
2526+ return ClientAllowed(address.to_v6().to_v4());
2527+
2528+ if (address == asio::ip::address_v4::loopback()
2529+ || address == asio::ip::address_v6::loopback())
No, it doesn’t. As for IPv6, that only has a single loopback address. (::1/128).
And while I agree that that ^^ code doesn’t address all loopback cases. It does address all loopback cases covered by the previous version of that code.
That being said, I’ll gladly add another commit to improve the IPv4 case (using a netmask check against 127.0.0.0/8).
This allows more flexibility in the RPC code, e.g. making it easier to
handle multiple simultaneous connections later on.
Currently asynchronous I/O is only used to listen for and accept
incoming connections. Asynchronous reading/writing is more involved.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
The RPC server now listens for, and handles, incoming connections on
both IPv4 as well as IPv6.
If available (and usable) it uses a dual IPv4/IPv6 socket on systems
that support it (e.g. Linux and BSDs) and falls back to separate
IPv4/IPv6 sockets on systems that don't (e.g. Windows).
Signed-off-by: Giel van Schijndel <me@mortis.eu>
Signed-off-by: Giel van Schijndel <me@mortis.eu>
Using this modification it should be relatively easy to, at a later
time, listen on multiple addresses (even Unix domain sockets should be
possible).
Signed-off-by: Giel van Schijndel <me@mortis.eu>
Signed-off-by: Giel van Schijndel <me@mortis.eu>
Signed-off-by: Giel van Schijndel <me@mortis.eu>
Conflicts:
src/bitcoinrpc.cpp
Signed-off-by: Giel van Schijndel <me@mortis.eu>
NOTE: This is required to be sure that we can properly shut down the RPC
thread.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
basic_socket_acceptor needs -lmswsock added to Windows builds:
@luke-jr it’s been long since I’ve done windows development, but don’t you mean ws2_32 ? And isn’t that linked to already? @Diapolo yes, I did read your comment. I however have a day job which doesn’t leave me much time during the week to reply immediately. So being patient enough to wait till the next weekend following your comment might be nice.
Then as for the actual content of your comment:
Your last commit is likely to break sth
Please elaborate, because I’ve carefully checked how my change would affect existing code. As far as I could see there should be no difference except the location from where the shutdown thread gets started.
or at least change the current behaviour once more
As explained above: shutdown behaviour should not be changed for existing/untouched code. It should only affect the termination of the RPC handling’s shutdown sequence.
I.e. the RPC code needs to be interrupted by a signal in order to terminate it. Setting a variable that can be polled (fShutdown) isn’t enough because we’re blocking until some kind of event (network I/O or internal operation on io_service or one of the sockets) occurs.
@muggenhor I didn’t want to hurry you the feedback of another dev would have been sufficient, too. I didn’t want to offend you. That said StartShutdown() is currently used in bitcoinrpc, main, net and test_bitcoin.
GUI:
StartShutdown() -> uiInterface.QueueShutdown() -> quit() for QCoreApplication (Qt event loop) -> Shutdown(NULL); in bitcoin.cpp (no exit here) -> return 0; (Bitcoin-Qt exit)
NOUI: StartShutdown() -> CreateThread(Shutdown, NULL); -> Shutdown(NULL) -> exit(0);
What happens if StartShutdown is called in e.g. net.cpp with your patch using the NOUI version? Perhaps you can explain to me the new flow with your patch for the NOUI version. I’m not that advanced with the boost signal thing ;). Just want to ensure nothing get’s broken.
@Diapolo as you correctly seem to have noticed nothing is changed for the GUI case (outside of the RPC code).
For the NOUI case the flow is changed to: StartShutdown() -> raise QueueShutdown() signal -> CreateThread(Shutdown, NULL); -> Shutdown(NULL) -> exit(0);
In addition to that, for both GUI/NOUI the RPC code now uses the QueueShutdown() signal to stop listening for new connections: QueueShutdown() signal -> for each listening socket as acceptor -> acceptor.cancel().
As for the CreateThread call. That’s registered with the QueueShutdown signal, so will get called immediately (along with other signal handlers) when the signal is raised/emitted/sent. (calling a signal is done by an immediate for-loop on all slots/handlers).
Use Boost's signal2 slot tracking mechanism to cancel any (still open)
listening sockets when receiving a shutdown signal.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
Signed-off-by: Giel van Schijndel <me@mortis.eu>