net: refactor the connection process. moving towards async connections. #10285

theuni · 2017-04-27T00:24:46Z

This untangles our current connection logic and moves toward async connections/resolves. Before merging, I'd like to construct a matrix of different connection configs, and make sure that each one has test coverage. I'm PRing now to get a feel for whether this is reviewable as-is or not.

In short, switch from "here's what we know, connect please!" to "walk through the individual connection steps in-order".

This moves the entire process into one function, but the next step will be to break out the individual steps (input checking, resolving, connection, node creation).

See the individual commit messages for more detail.

No functional change, just preparation for the following commits.

ConnectNode is only called by OpenNetworkConnection, and only in one place. Combine them for preparation of splitting them into functions that are friendlier for callbacks.

The next commits will move the connection logic out of netbase into net.

This is the next step towards asynchronous connections. The current code confuses connection steps and tends to obfuscate what's going on. Here, I've tried to create individual stages. The logic has all moved into a single function for now: - Global checks: Should any connection be attempted? - Input checks: is there something valid to connect to? - Resolving: If the address string may be valid, attempt to resolve it (if allowed by the user's dns settings and proxy config) - Proxy config - Socket creation - Connect to the destination - If proxied, attempt the remote connection - Create the node with the resulting connection With this done, resolves and connections can trigger callbacks so that a connection attempt requires no return value. At that point, they can be made asynchronous via libevent.

gmaxwell · 2017-04-27T00:31:10Z

Vague Concept ACK.

laanwj · 2017-05-02T12:32:03Z

utACK, going to test this

TheBlueMatt

Review turned up a few bugs that should be fixed, though most of them not your fault :/.

TheBlueMatt · 2017-05-02T14:36:07Z

src/net.cpp

+    if (!fNetworkActive) {
+        return false;
+    }
+    if (IsLocal(addrConnect)) {


Shouldnt this only be if !pszDest?

yes, thanks. Will fix.

Note: Fixed in 1d94e2e.

TheBlueMatt · 2017-05-02T18:17:28Z

src/net.cpp


-        addrman.Attempt(addrConnect, fCountFailure);
+    if (!strDest.empty() && addrDest.IsValid()) {


nit: Move this into the previous block, and maybe move the addrDest.IsValid() check into it as well. Then you can always require addrDest.IsValid() for non-named addresses as well, which seems reasonable.

Generally the point here is to have top-level ifs like if (strDest.empty()) and then put the logic inside of that, instead of repeated ifs for similar branches which are just confusing.

I avoided coalescing on purpose. The current code does a bunch of steps at once, depending on what the input is. That's what I'm trying to undo here.

The previous block handles resolving, and will be moved out to a new function as a next step. By keeping the connection flow separated like this, it's trivial to factor out that way.

Ahh, OK, didnt realize that the next step would be to pull it out.

By keeping the connection flow separated like this, it's trivial to factor out that way.

Probably there is something going over my head, but I don't understand the reasoning.

Currently this does:

if (condition) { // bunch of code } else { // other code } if (equivalent condition) { // code that you want to factor out } if (equivalent condition) { // more code that you want to factor out }

I don't see why it would be any harder to refactor later with:

if (condition) { // bunch of code // code that you want to factor out // more code that you want to factor out } else { // other code }

While it may make sense to lump a few actions together if they meet the same conditions now because a connection is just one long synchronous operation, my intention is to clarify which conditions really need to be met at which stage, so that we can break that operation up into smaller asynchronous ones.

Some of these tests (like whether or not we resolved successfully) will just move in an obvious way to callbacks, so I guess it makes sense to combine them here. Others though, will need to be re-checked on separate threads.

I'll clarify this in the code by setting some variables rather than repeating conditions, so that it's more clear what the intent is.

I'll rebase while I'm at it.

TheBlueMatt · 2017-05-02T19:15:54Z

src/netbase.cpp

@@ -445,6 +439,19 @@ bool static ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocketRe
    if (!SetSocketNonBlocking(hSocket, true))
        return error("ConnectSocketDirectly: Setting socket to non-blocking failed, error %s\n", NetworkErrorString(WSAGetLastError()));


nit: needs a string change here for __func__.

Oh, also think we need a CloseSocket here.

Will fix func.

SetSocketNonBlocking should take care of closing the socket, but an explicit one here wouldn't hurt either.

CloseSocket is really annoying. I'll RAII it directly after this PR.

Good, CloseSocket were half the issues here :(

Isn't seems strange for SetSocketNonBlocking to try to destroy the socket if it doesn't succeed. Maybe consider removing CloseSocket calls from SetSocketNonBlocking after adding the RAII here.

TheBlueMatt · 2017-05-02T19:25:03Z

src/net.cpp

+    addrman.Attempt(addrDest, fCountFailure);
+
+    if (!connected) {
+        return false;


Not strictly your fault, but I think you're exposing a bug in Socks5 here - Socks5 fails to CloseSocket() prior to returning false in one place (Proxy username or password too long), so I think it'd be better to just CloseSocket here instead of further down to make it harder to miss such things.

+1 for the SocketCloser

TheBlueMatt · 2017-05-02T19:50:06Z

src/netbase.cpp

+    struct sockaddr_storage sockaddr;
+    socklen_t len = sizeof(sockaddr);
+    if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) {
+        LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToString());


Need a CloseSocket here (but, really, lets just move all the CloseSockets up to OpenNetworkConnection post-CreateSocket instead of them being sprinkled all over everywhere?).

Ugh. I'll see how much work it would be to RAII it as part of this pull.

Note: fixed in 34133bd.

- Only check IsLocal if pszDest is unset - Fix function name string in CreateSocket

TheBlueMatt · 2017-05-05T19:29:12Z

src/net.cpp

-        addrman.Attempt(addrConnect, fCountFailure);
+    // Create socket and make sure that it is closed in the event of an error
+    SOCKET hSocket = INVALID_SOCKET;
+    CSocketCloser sockCloser(hSocket);


It seems strange to create the CSocketCloser and then give it a reference to the socket - why not just have the CSocketCloser hold the SOCKET variable (publicly) and then set it to INVALID_SOCKET upon close/release (and return it from release) to make it feel more like a unique_ptr?

It seems strange to create the CSocketCloser and then give it a reference to the socket

It is strange, just like CloseSocket taking a reference is strange. But I'd think as long as one of these is taking a reference, the other probably should as well for consistency.

I gave it a reference because several calls further down the chain set the value to -1. For example, if ConnectSocketDirectly() fails. If storing a value, CSocketCloser would attempt to close it again at destruction if we weren't careful about resetting it, which would defeat the point of using RAII.

The alternative would be to audit for CloseSocket()s (or make a full-blown RAII CSocket with Close() as a member), which would make for a much larger diff.

ryanofsky

utACK 1d94e2e.

Would be nice to squash review fixes into place.

I don't think this PR is a pure improvement because OpenNetworkConnection is now a big monolith, but there are nice changes here and presumably there will be more cleanup in the future.

ryanofsky · 2017-05-10T21:23:47Z

src/net.cpp

@@ -340,7 +340,7 @@ bool CConnman::CheckIncomingNonce(uint64_t nonce)
    return true;
 }

-CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure)
+CNode* CConnman::ConnectNode(const CAddress& addrConnect, const char *pszDest, bool fCountFailure)


In commit "net: pass a reference into ConnectNode and copy it there" (8245107)

Corresponding change to net.h is missing so this commit doesn't compile.

ryanofsky · 2017-05-10T21:33:06Z

src/net.cpp

+    if (!fNetworkActive) {
+        return false;
+    }
+    if (IsLocal(addrConnect)) {


Note: Fixed in 1d94e2e.

ryanofsky · 2017-05-10T21:41:58Z

src/netbase.cpp

@@ -445,6 +439,19 @@ bool static ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocketRe
    if (!SetSocketNonBlocking(hSocket, true))
        return error("ConnectSocketDirectly: Setting socket to non-blocking failed, error %s\n", NetworkErrorString(WSAGetLastError()));


Isn't seems strange for SetSocketNonBlocking to try to destroy the socket if it doesn't succeed. Maybe consider removing CloseSocket calls from SetSocketNonBlocking after adding the RAII here.

ryanofsky · 2017-05-10T21:43:28Z

src/netbase.cpp

+    struct sockaddr_storage sockaddr;
+    socklen_t len = sizeof(sockaddr);
+    if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) {
+        LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToString());


Note: fixed in 34133bd.

ryanofsky · 2017-05-10T21:49:36Z

src/netbase.cpp

+    return true;
+}
+
+bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocket, int nTimeout)


In commit "net: split socket creation out of connection"

This commit drops static, but the function isn't exposed publicly in a header until the next commit.

ryanofsky · 2017-05-10T22:34:38Z

src/net.cpp

-        addrman.Attempt(addrConnect, fCountFailure);
+    // Create socket and make sure that it is closed in the event of an error
+    SOCKET hSocket = INVALID_SOCKET;
+    CSocketCloser sockCloser(hSocket);


It seems strange to create the CSocketCloser and then give it a reference to the socket

It is strange, just like CloseSocket taking a reference is strange. But I'd think as long as one of these is taking a reference, the other probably should as well for consistency.

ryanofsky · 2017-05-10T22:46:03Z

src/net.cpp


-        addrman.Attempt(addrConnect, fCountFailure);
+    if (!strDest.empty() && addrDest.IsValid()) {


By keeping the connection flow separated like this, it's trivial to factor out that way.

Probably there is something going over my head, but I don't understand the reasoning.

Currently this does:

if (condition) { // bunch of code } else { // other code } if (equivalent condition) { // code that you want to factor out } if (equivalent condition) { // more code that you want to factor out }

I don't see why it would be any harder to refactor later with:

if (condition) { // bunch of code // code that you want to factor out // more code that you want to factor out } else { // other code }

theuni · 2017-05-18T01:30:12Z

Closing this for now. I'm working on getting this fully refactored with libevent instead.

Breaking this out and serializing the logic was very helpful as an intermediary step, but I now agree that it doesn't make much sense to review/merge those changes alone.

b887676 net: remove now-unused functions (Cory Fields) 45fd754 net: remove now-superfluous numeric resolve (Cory Fields) 2416dd7 net: separate resolving and conecting (Cory Fields) Pull request description: This is a greatly simplified version of #10285, which only aims to address async resolving. It essentially breaks up two wrapper functions for things only used in one place (ConnectSocketDirectly/ConnectThroughProxy) in favor of calling them directly. This allows us to fully handle resolves before attempting a connection, as is necessary for async connections. As a bonus, I believe the logic is now much easier to follow than before. Tree-SHA512: f03f618107379edf3efe2a9f3e3677e8f075017ab140a0b4fdc3b8263e6beff148d55256263ab10bc2125ef089ca68e0d8e865beeae176f1eca544e769c976d3

b887676 net: remove now-unused functions (Cory Fields) 45fd754 net: remove now-superfluous numeric resolve (Cory Fields) 2416dd7 net: separate resolving and conecting (Cory Fields) Pull request description: This is a greatly simplified version of bitcoin#10285, which only aims to address async resolving. It essentially breaks up two wrapper functions for things only used in one place (ConnectSocketDirectly/ConnectThroughProxy) in favor of calling them directly. This allows us to fully handle resolves before attempting a connection, as is necessary for async connections. As a bonus, I believe the logic is now much easier to follow than before. Tree-SHA512: f03f618107379edf3efe2a9f3e3677e8f075017ab140a0b4fdc3b8263e6beff148d55256263ab10bc2125ef089ca68e0d8e865beeae176f1eca544e769c976d3

b887676 net: remove now-unused functions (Cory Fields) 45fd754 net: remove now-superfluous numeric resolve (Cory Fields) 2416dd7 net: separate resolving and conecting (Cory Fields) Pull request description: This is a greatly simplified version of bitcoin#10285, which only aims to address async resolving. It essentially breaks up two wrapper functions for things only used in one place (ConnectSocketDirectly/ConnectThroughProxy) in favor of calling them directly. This allows us to fully handle resolves before attempting a connection, as is necessary for async connections. As a bonus, I believe the logic is now much easier to follow than before. Tree-SHA512: f03f618107379edf3efe2a9f3e3677e8f075017ab140a0b4fdc3b8263e6beff148d55256263ab10bc2125ef089ca68e0d8e865beeae176f1eca544e769c976d3 ConnectSocket -> ConnectSocketDirectly Signed-off-by: Pasta <pasta@dashboost.org>

theuni added 6 commits April 21, 2017 18:18

net: pass a reference into ConnectNode and copy it there

8245107

No functional change, just preparation for the following commits.

net: combine OpenNetworkConnection and ConnectNode

4044e82

ConnectNode is only called by OpenNetworkConnection, and only in one place. Combine them for preparation of splitting them into functions that are friendlier for callbacks.

net: split socket creation out of connection

c057713

net: expose a few functions/structs from netbase

5a5272a

The next commits will move the connection logic out of netbase into net.

net: remove now-unused functions

235dc01

fanquake added P2P Refactoring labels Apr 27, 2017

TheBlueMatt reviewed May 2, 2017

View reviewed changes

theuni added 2 commits May 2, 2017 17:31

net: Add a quick RAII socket closer

34133bd

squashme: review fixups

1d94e2e

- Only check IsLocal if pszDest is unset - Fix function name string in CreateSocket

TheBlueMatt reviewed May 5, 2017

View reviewed changes

ryanofsky reviewed May 10, 2017

View reviewed changes

theuni closed this May 18, 2017

theuni mentioned this pull request Jun 24, 2017

net: split resolve out of connect #10663

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021


		addrman.Attempt(addrConnect, fCountFailure);
		if (!strDest.empty() && addrDest.IsValid()) {

		@@ -445,6 +439,19 @@ bool static ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocketRe
		if (!SetSocketNonBlocking(hSocket, true))
		return error("ConnectSocketDirectly: Setting socket to non-blocking failed, error %s\n", NetworkErrorString(WSAGetLastError()));

net: refactor the connection process. moving towards async connections. #10285

net: refactor the connection process. moving towards async connections. #10285

Uh oh!

Conversation

theuni commented Apr 27, 2017

Uh oh!

gmaxwell commented Apr 27, 2017

Uh oh!

laanwj commented May 2, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented May 18, 2017

Uh oh!

Uh oh!