netbase: Make SOCKS5 negotiation interruptible #4869

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2014_09_socks5_negotiation changing 2 files +70 −22

laanwj commented at 11:59 AM on September 8, 2014: member
Avoids that SOCKS5 negotiation will hold up the shutdown process.
- Sockets can stay in non-blocking mode, no need to switch it on/off anymore
- Adds a timeout (20 seconds) on SOCKS5 negotiation. This should be enough for even Tor to get a connection to a hidden service, and avoids blocking the opencon thread indefinitely on a hanging proxy.
Fixes #2954.

Should also solve #4502 by making sure that the socket for GetMyExternalIP2 stays in non-blocking mode (would be even better to use InterruptibleRecv in RecvLine for proper timeout/interruption handling, but this is going away anyway...).
laanwj renamed this:
~~2014 09 socks5 negotiation~~
netbase: Make SOCKS5 negotiation interruptible
on Sep 8, 2014
laanwj commented at 12:06 PM on September 8, 2014: member

@diapolo can you help test this in windows?
laanwj added the label P2P on Sep 8, 2014
in src/netbase.cpp:None in 5df9b09c8f outdated
```
 265 | +            len -= ret;
 266 | +            data += ret;
 267 | +        } else if (ret == 0) // Unexpected disconnection
 268 | +        {
 269 | +            return false;
 270 | +        } else { // Other error or blocking
```
sipa commented at 1:09 PM on September 8, 2014:

Mix of { on the same line and on the next line?

laanwj commented at 1:11 PM on September 8, 2014:

What should it be?

sipa commented at 1:24 PM on September 8, 2014:

To follow coding-style.md: on the same line for everything inside code bodies. But at least consistent within one function...

laanwj commented at 1:38 PM on September 8, 2014:

Is it possible to run just a function through the clang formatter? Would make sense to introduce new code with the right formatting.

laanwj commented at 1:41 PM on September 8, 2014:

Ok, did this by reformatting the whole file and just copying the function afterwards.

sipa commented at 1:55 PM on September 8, 2014:

I don't see anything?
sipa commented at 1:10 PM on September 8, 2014: member

ut ACK
laanwj force-pushed on Sep 8, 2014
Diapolo commented at 2:46 PM on September 8, 2014: none

Yeah, I can take a look at this tomorrow.
laanwj force-pushed on Sep 8, 2014
in src/netbase.cpp:None in 1e213c8c11 outdated
```
 260 | +    while (len > 0 && curTime < endTime) {
 261 | +        ssize_t ret = recv(hSocket, data, len, 0); // Optimistically try the recv first
 262 | +        if (ret > 0) {
 263 | +            len -= ret;
 264 | +            data += ret;
 265 | +        } else if (ret == 0) { // Unexpected disconnection
```
Diapolo commented at 8:57 AM on September 9, 2014:

MSDN sais this is a gracefully disconnect: http://msdn.microsoft.com/de-de/library/windows/desktop/ms740121%28v=vs.85%29.aspx

Doesn't change a thing code wise, but I'd suggest changing the comment accordingly.

laanwj commented at 9:01 AM on September 9, 2014:

Graceful maybe, but still unexpected.

Diapolo commented at 9:10 AM on September 9, 2014:

Yeah :-P.

in src/netbase.cpp:None in 1e213c8c11 outdated

 269 | +            if (nErr == WSAEINPROGRESS || nErr == WSAEWOULDBLOCK || nErr == WSAEINVAL) {
 270 | +                struct timeval tval = MillisToTimeval(std::min(endTime - curTime, maxWait));
 271 | +                fd_set fdset;
 272 | +                FD_ZERO(&fdset);
 273 | +                FD_SET(hSocket, &fdset);
 274 | +                int nRet = select(hSocket + 1, &fdset, NULL, NULL, &tval);

Diapolo commented at 9:08 AM on September 9, 2014:

You could leave nRet out by using the select() directly in the if-clause.

Diapolo commented at 9:10 AM on September 9, 2014: none

I'm currently testing this now...

Edit: I added a debugging line and currently the only (non-)error that is logged is this one: InterruptibleRecv: Socket error on recv: Ein nicht blockierender Socketvorgang konnte nicht sofort ausgeführt werden. (10035).

I also can confirm that shutdown is a lot quicker now when using Tor as proxy. Edit 2: I observed a situation, where shutdown hung and I had to force a close of bitcoin-qt.exe. I'm not sure if it was related to this pull.
laanwj commented at 3:05 PM on September 9, 2014: member

So shutdown still hangs after this? Did it hang on waiting for the network thread or something else?
Diapolo commented at 5:29 PM on September 9, 2014: none

@laanwj It doesn't hang generally, it was faster the first few times I tried a shutdown. But I had one case, where it would never shutdown and I had to kill the process. As I said I'm not yet sure it's related only to this pull.
laanwj commented at 8:08 AM on September 10, 2014: member

Ok, next time you get the hang please debug where it happens. We can't move forward here if we don't have more information.
Diapolo commented at 9:03 AM on September 10, 2014: none

@laanwj I know this is not helpful yet, because I dunno where that behaviour came from, but I'm testing it and reported that case ;).

netbase: Make SOCKS5 negotiation interruptible

Avoids that SOCKS5 negotiation will hold up the shutdown process.

- Sockets can stay in non-blocking mode, no need to switch it on/off
  anymore
- Adds a timeout (20 seconds) on SOCK5 negotiation. This should be
  enough for even Tor to get a connection to a hidden service, and
  avoids blocking the opencon thread indefinitely on a hanging proxy.

Fixes #2954.

6050ab6855

laanwj force-pushed on Sep 10, 2014
BitcoinPullTester commented at 9:47 AM on September 10, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4869_6050ab685553c7312ef105d2c4a5230c3fcf4002/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
Diapolo commented at 11:47 AM on September 14, 2014: none

I was not able to reproduce the behaviour, it worked like a charm the last days.

ACK
jgarzik commented at 1:35 AM on September 15, 2014: contributor

ut ACK
laanwj merged this on Sep 15, 2014
laanwj closed this on Sep 15, 2014
laanwj referenced this in commit 327dcfece7 on Sep 15, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#2954 opencon thread massively slows down client shutdown?#4502 GetMyExternalIP hangs shutdown when waiting for response