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

theuni commented at 12:24 AM on April 27, 2017: member

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.

net: pass a reference into ConnectNode and copy it there

No functional change, just preparation for the following commits.

82451074c2

net: combine OpenNetworkConnection and ConnectNode

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.

4044e8220d

net: split socket creation out of connection c0577133cb

net: expose a few functions/structs from netbase

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

5a5272a5b0

net: refactor the outgoing connection process

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.

ae14fda915

net: remove now-unused functions 235dc01fc0

fanquake added the label P2P on Apr 27, 2017

fanquake added the label Refactoring on Apr 27, 2017

gmaxwell commented at 12:31 AM on April 27, 2017: contributor

Vague Concept ACK.

laanwj added this to the "Blockers" column in a project

laanwj commented at 12:32 PM on May 2, 2017: member

utACK, going to test this

in src/net.cpp:356 in 4044e8220d outdated

 362 | +        return false;
 363 | +    }
 364 | +    if (!fNetworkActive) {
 365 | +        return false;
 366 | +    }
 367 | +    if (IsLocal(addrConnect)) {

TheBlueMatt commented at 2:36 PM on May 2, 2017:

Shouldnt this only be if !pszDest?

theuni commented at 8:05 PM on May 2, 2017:

yes, thanks. Will fix.

ryanofsky commented at 9:33 PM on May 10, 2017:

Note: Fixed in 1d94e2ebe470cb8842223520a0c57513ca77e940.

in src/net.cpp:406 in 235dc01fc0 outdated

 436 |              }
 437 |          }
 438 | +    }
 439 |  
 440 | -        addrman.Attempt(addrConnect, fCountFailure);
 441 | +    if (!strDest.empty() && addrDest.IsValid()) {

TheBlueMatt commented at 6:17 PM on May 2, 2017:

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.

TheBlueMatt commented at 6:28 PM on May 2, 2017:

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.

theuni commented at 8:02 PM on May 2, 2017:

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.

TheBlueMatt commented at 8:13 PM on May 2, 2017:

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

ryanofsky commented at 10:46 PM on May 10, 2017:

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 commented at 4:53 AM on May 11, 2017:

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.

in src/netbase.cpp:440 in 235dc01fc0 outdated

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

TheBlueMatt commented at 7:15 PM on May 2, 2017:

nit: needs a string change here for __func__.

TheBlueMatt commented at 7:49 PM on May 2, 2017:

Oh, also think we need a CloseSocket here.

theuni commented at 8:08 PM on May 2, 2017:

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.

TheBlueMatt commented at 8:13 PM on May 2, 2017:

Good, CloseSocket were half the issues here :(

ryanofsky commented at 9:41 PM on May 10, 2017:

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.

in src/net.cpp:473 in 235dc01fc0 outdated

 515 |  
 516 | -    return NULL;
 517 | +    addrman.Attempt(addrDest, fCountFailure);
 518 | +
 519 | +    if (!connected) {
 520 | +        return false;

TheBlueMatt commented at 7:25 PM on May 2, 2017:

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.

theuni commented at 8:06 PM on May 2, 2017:

Agreed.

laanwj commented at 12:45 PM on May 3, 2017:

+1 for the SocketCloser

in src/netbase.cpp:451 in 235dc01fc0 outdated

 446 | +bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocket, int nTimeout)
 447 | +{
 448 | +    struct sockaddr_storage sockaddr;
 449 | +    socklen_t len = sizeof(sockaddr);
 450 | +    if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) {
 451 | +        LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToString());

TheBlueMatt commented at 7:50 PM on May 2, 2017:

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

theuni commented at 8:10 PM on May 2, 2017:

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

ryanofsky commented at 9:43 PM on May 10, 2017:

Note: fixed in 34133bd6659ed02002faad757da38562df0730ae.

TheBlueMatt commented at 7:54 PM on May 2, 2017: member

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

net: Add a quick RAII socket closer 34133bd665

squashme: review fixups

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

1d94e2ebe4

in src/net.cpp:428 in 1d94e2ebe4

 469 | -        // If connecting to the node failed, and failure is not caused by a problem connecting to
 470 | -        // the proxy, mark this as an attempt.
 471 | -        addrman.Attempt(addrConnect, fCountFailure);
 472 | +    // Create socket and make sure that it is closed in the event of an error
 473 | +    SOCKET hSocket = INVALID_SOCKET;
 474 | +    CSocketCloser sockCloser(hSocket);

TheBlueMatt commented at 7:29 PM on May 5, 2017:

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?

ryanofsky commented at 10:34 PM on May 10, 2017:

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.

theuni commented at 11:51 PM on May 10, 2017:

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.

in src/net.cpp:343 in 82451074c2 outdated

 339 | @@ -340,7 +340,7 @@ bool CConnman::CheckIncomingNonce(uint64_t nonce)
 340 |      return true;
 341 |  }
 342 |  
 343 | -CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure)
 344 | +CNode* CConnman::ConnectNode(const CAddress& addrConnect, const char *pszDest, bool fCountFailure)

ryanofsky commented at 9:23 PM on May 10, 2017:

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

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

in src/netbase.cpp:452 in c0577133cb outdated

 444 | @@ -445,6 +445,22 @@ bool static ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocketRe
 445 |      if (!SetSocketNonBlocking(hSocket, true))
 446 |          return error("ConnectSocketDirectly: Setting socket to non-blocking failed, error %s\n", NetworkErrorString(WSAGetLastError()));
 447 |  
 448 | +    hSocketRet = hSocket;
 449 | +    return true;
 450 | +}
 451 | +
 452 | +bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocket, int nTimeout)

ryanofsky commented at 9:49 PM on May 10, 2017:

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 commented at 10:53 PM on May 10, 2017: member

utACK 1d94e2ebe470cb8842223520a0c57513ca77e940.

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.

theuni commented at 1:30 AM on May 18, 2017: member

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.

theuni closed this on May 18, 2017

laanwj removed this from the "Blockers" column in a project

laanwj referenced this in commit 9d31ed2e69 on Sep 28, 2017

PastaPastaPasta referenced this in commit 3a6067ada0 on Dec 22, 2019

PastaPastaPasta referenced this in commit c637771b9d on Jan 2, 2020

PastaPastaPasta referenced this in commit efa150fb3e on Jan 4, 2020

PastaPastaPasta referenced this in commit 58c58e1f12 on Jan 12, 2020

PastaPastaPasta referenced this in commit d17fbc84ba on Jan 12, 2020

PastaPastaPasta referenced this in commit 25e686e7f1 on Jan 12, 2020

PastaPastaPasta referenced this in commit 09ca733913 on Jan 12, 2020

PastaPastaPasta referenced this in commit f411c5cd94 on Jan 12, 2020

PastaPastaPasta referenced this in commit 19180b3357 on Jan 12, 2020

ckti referenced this in commit edf5d1faf0 on Mar 28, 2021

gades referenced this in commit a6d5b1a704 on Jun 25, 2021

DrahtBot locked this on Sep 8, 2021