Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

IPv6 RPC using asynchronously accepted connections #457

Merged
merged 11 commits into from Jun 27, 2012

Conversation

Projects
None yet
6 participants
Contributor

muggenhor commented Aug 10, 2011

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.

Contributor

gavinandresen commented Aug 10, 2011

I'm concerned this might make RPC code that implicitly assumes the RPC is single-threaded deadlock or crash. How much testing did you do-- has this been tested on an in-production, high-RPC-traffic server?

Contributor

muggenhor commented Aug 10, 2011

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).

Contributor

muggenhor commented Aug 10, 2011

PS The main reason for using asynchronous I/O is to allow binding to multiple addresses for RPC without requiring one thread for every socket. This is what enables a dual IPv4/IPv6 stack.

Contributor

gavinandresen commented Aug 11, 2011

I don't know nuthin about IPv6/boost::asio stuff. General comment is it seems like this maybe should be part of a larger "support IPv6" branch.

Contributor

muggenhor commented Aug 11, 2011

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.

Contributor

muggenhor commented Sep 4, 2011

I've rebased the branch against master to make it easy to merge in.

Member

luke-jr commented Oct 12, 2011

This conflicts with threaded JSON-RPC which is needed by many people. Can you make an IPv6-only version?

Contributor

jgarzik commented Dec 19, 2011

agree w/ first half of luke-jr's comment. second half... not sure we want an IPv6-only version?

Member

luke-jr commented Dec 19, 2011

I meant a patch that only adds IPv6, without the conflicting async stuff.

Contributor

muggenhor commented Dec 19, 2011

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).

Member

luke-jr commented Dec 19, 2011

Can we do async for listening only, then? Threads are needed for actual RPC calls since some may block.

Contributor

muggenhor commented Dec 20, 2011

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.

Member

luke-jr commented Dec 20, 2011

I dislike the fact that a 'getwork' call will block all other JSON-RPC until it completes.

Contributor

muggenhor commented Dec 21, 2011

Yes, but that is unrelated to my patch.

The alternate implementation of IPv6 support I'm thinking of would look somewhat like this pseudocode:

  • create listener sockets (IPv4 and IPv6)
  • asynchronously accept a connection on both listeners (acceptor's in Asio's terminology)
  • from the accept callbacks: place the newly connected socket in a queue, then start a new async accept op

the mainloop would then be this:

  • wait for a single event to occur (io_service.run_one())
  • handle all connections in the queue until queue is empty
  • restart loop

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.

Member

luke-jr commented Dec 21, 2011

It's related, because your patch conflicts with it. Instead of conflicting, why not implement IPv6 RPC on top of the existing multithreaded JSON-RPC branch (#568)?

Contributor

muggenhor commented Dec 22, 2011

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.

Contributor

muggenhor commented Dec 24, 2011

Current branch is on top of #568. I've used the approach outlined above (using a connection queue).

Member

luke-jr commented Dec 24, 2011

ACK: Tested fine for me in 'next-test'

Member

luke-jr commented Feb 5, 2012

For some reason, if -rpcallowip is used, it sees local connections as ::ffff:127.0.0.1 and sends a 403 instead of allowing the connection.

(side note: #568 has been rebased)

Contributor

jgarzik commented May 8, 2012

Request rebase on top of #1101... we certainly do want to support IPv6 RPC.

Contributor

muggenhor commented May 13, 2012

@jgarzik I'll work on updating this pull request next Thursday (Ascension Day, national holiday so I'll have some time off).

Contributor

jgarzik commented May 13, 2012

Thanks!

Note that pull #1101 is now upstream, and will be in upcoming version 0.7

Member

luke-jr commented May 13, 2012

Please don't forget to fix the -rpcallowip issue.

Contributor

muggenhor commented May 20, 2012

@luke-jr This also contains a change (in 652eebf08e7f0e32d686d4e36475742fa27f71cc) to treat IPv4-mapped IPv6 addresses (::127.0.0.1 is one) as IPv4 addresses.

@luke-jr luke-jr and 1 other commented on an outdated diff May 20, 2012

src/bitcoinrpc.cpp
{
- if (strAddress == asio::ip::address_v4::loopback().to_string())
+ // Make sure that IPv4-compatible and IPv4-mapped IPv6 addresses are treated as IPv4 addresses
+ if (address.is_v6()
+ && (address.to_v6().is_v4_compatible()
+ || address.to_v6().is_v4_mapped()))
+ return ClientAllowed(address.to_v6().to_v4());
+
+ if (address == asio::ip::address_v4::loopback()
+ || address == asio::ip::address_v6::loopback())
@luke-jr

luke-jr May 20, 2012

Member

This strikes me as flawed by design, though perhaps that's inherited from boost... does it not have any kind of "is loopback" method?
Even IPv4 has 16,777,216 unique loopback addresses.

@muggenhor

muggenhor May 20, 2012

Contributor

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).

Contributor

muggenhor commented May 21, 2012

I believe that this pull request is ready for merging.

muggenhor added some commits Aug 10, 2011

Use asynchronous I/O to handle RPC requests
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>
Add dual IPv4/IPv6 stack support to the RPC server
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>
Allow clients on the IPv6 loopback as well
Signed-off-by: Giel van Schijndel <me@mortis.eu>
Generalise RPC connection handling code to allow more listening sockets
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>
Allow all addresses on the loopback subnet (127.0.0.0/8) not just 127…
….0.0.1

Signed-off-by: Giel van Schijndel <me@mortis.eu>
Use the QueueShutdown signal to stop accepting new RPC connections
Signed-off-by: Giel van Schijndel <me@mortis.eu>
Owner

sipa commented Jun 9, 2012

ACK

muggenhor added some commits Jun 17, 2012

Merge branch 'master' into async-ipv6-rpc
Conflicts:
	src/bitcoinrpc.cpp

Signed-off-by: Giel van Schijndel <me@mortis.eu>
*Always* send a shutdown signal to enable custom shutdown actions
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>

Diapolo commented Jun 17, 2012

NACK until the last commit is clarified.

@muggenhor Wait, what are you doing there to the shutdown ... we had a long discussion and merged a patch a few days ago. Your last commit is likely to break sth. or at least change the current behaviour once more, see #1439.

Member

luke-jr commented Jun 22, 2012

basic_socket_acceptor needs -lmswsock added to Windows builds:

  • bitcoin-qt.pro
  • src/makefile.linux-mingw
  • src/makefile.mingw

Diapolo commented Jun 22, 2012

Did anyone mind reading my comment above lukes...? I'm sure the last commit can cause trouble.

Contributor

muggenhor commented Jun 24, 2012

@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.

Diapolo commented Jun 24, 2012

@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.

Contributor

muggenhor commented Jun 24, 2012

@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).

Owner

sipa commented Jun 24, 2012

It does seem to simplify the shutdown code. @Diapolo: any reason to assume things will break with this patch?

Both bitcoin-qt and bitcoind seem to shutdown fine with this, via RPC stop, UI quit, or SIGINT.

Member

luke-jr commented Jun 24, 2012

@muggenhor basic_socket_acceptor uses AcceptEx, which is defined in mswsock

Contributor

muggenhor commented Jun 24, 2012

@luke-jr basic_socket_acceptor is already used in mainline, so the problem should exist already. Regardless, I've fixed it as well in my branch.

muggenhor added some commits Jun 24, 2012

Cancel outstanding listen ops for RPC when shutting down
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>
On Windows link with `mswsock`, it being required (indirectly) by RPC…
… code

Signed-off-by: Giel van Schijndel <me@mortis.eu>

Diapolo commented Jun 24, 2012

I have no further doubts after reading the explanations above. Thanks for clarification!

sipa added a commit that referenced this pull request Jun 27, 2012

Merge pull request #457 from muggenhor/async-ipv6-rpc
IPv6 RPC using asynchronously accepted connections

@sipa sipa merged commit 4a52c18 into bitcoin:master Jun 27, 2012

coblee pushed a commit to litecoin-project/litecoin that referenced this pull request Jul 17, 2012

Merge pull request #457 from muggenhor/async-ipv6-rpc
IPv6 RPC using asynchronously accepted connections

zathras-crypto pushed a commit to zathras-crypto/omnicore that referenced this pull request Feb 28, 2017

Merge #457: Add seed blocks for 440,000 to 450,000
be545e1 Update MAX_SEED_BLOCKS value (thanks @dexX7!!!) (Zathras)
519bc88 Add seed blocks for 440,000 to 450,000 (Zathras)

deadalnix pushed a commit to deadalnix/bitcoin that referenced this pull request Apr 27, 2017

Merge pull request #457 from ptschip/dev_expedited
Create expedited.cpp and move expedited code there
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment