select()'s first argument should be zero, if no file descriptors are selected #1786

pull jgarzik wants to merge 1 commits into bitcoin:master from jgarzik:select-fix changing 1 files +7 −3

jgarzik commented at 3:28 AM on September 5, 2012: contributor

The most portable invocation for the zero-fds case, a widely used portable hack for sleeping, generally involves passing NULL arguments when the fd sets are unused. This form is used here.

Intended to supercede #1772.
laanwj commented at 5:26 AM on September 5, 2012: member

Yes, adding an extra flag should solve the problem too. ACK
Diapolo commented at 6:23 AM on September 5, 2012: none

This does not fix 10022 spam on Windows, as WSAEINVAL (10022) is caused by: The time-out value is not valid, or all three descriptor parameters were null..

But I think it is another fix worth to merge in addition to #1772, so ACK.

in src/net.cpp:None in 65dfed7eed outdated

 781 | +
 782 |          vnThreadsRunning[THREAD_SOCKETHANDLER]--;
 783 | -        int nSelect = select(hSocketMax + 1, &fdsetRecv, &fdsetSend, &fdsetError, &timeout);
 784 | +        if (have_fds)
 785 | +            nSelect = select(hSocketMax + 1,
 786 | +                             &fdsetRecv, &fdsetSend, &fdsetError, &timeout);

Diapolo commented at 6:25 AM on September 5, 2012:

What adds such a line-break to readability?

jgarzik commented at 6:34 AM on September 5, 2012: contributor

@Diapolo so you actually tested this on Windows, for the net and !net cases? I'm not sure the Windows expected behavior matches what you describe...
laanwj commented at 6:38 AM on September 5, 2012: member

Indeed, that WSAEINVAL: the time-out value is not valid, or all three descriptor parameters were null comes from the MS documentation http://msdn.microsoft.com/en-us/library/windows/desktop/ms740141(v=vs.85).aspx

So yes on Windows you still need the check to prevent error spam. It appears that windows does not allow for using select as simply a timeout. However, now you can check for have_fds instead of hSocketMax!=0, which is at least correct.
jgarzik commented at 6:42 AM on September 5, 2012: contributor

Yes, I understand it comes from the documentation, but is it actually seen after applying this patch?
Diapolo commented at 6:49 AM on September 5, 2012: none

The error 10022 is still logged with this and without my patch, as select() returns WSAEINVAL. I tried it :). @laanwj Yes, a check for have_fds is working, I can update my pull after this one is in.
laanwj commented at 7:14 AM on September 5, 2012: member

It makes sense that it still occurs because windows ignores the nfds argument. So this change is a no-op from the viewpoint of winsock.

We could spare ourselves worrying about all these kinds of low-level OS details if we used an abstraction such as boost::asio (which will also use more efficient mechanisms such as epoll on platforms that support them).
Diapolo commented at 7:24 AM on September 5, 2012: none

That is what I tried to say @jgarzik, his patch does not prevent the 10022 spam, I read the select() documentation on MSDN more than once to understand what is happening ^^.

I rather sure most part of that network code are still from Satoshi (at least he was the one who merged them to Github at first).
select(): Use precise fd presence check, rather than imprecise hSocketMax test 2387944782
jgarzik commented at 8:04 PM on September 5, 2012: contributor
1. Updated to always pass fd sets to select(), even when zero fd's are set.
2. Use precise have_fds test before squawking on socket error.
in src/net.cpp:None in 2387944782
```
 788 |          {
 789 | -            int nErr = WSAGetLastError();
 790 | -            if (hSocketMax != INVALID_SOCKET)
 791 | +            if (have_fds)
 792 |              {
 793 | +                int nErr = WSAGetLastError();
```
Diapolo commented at 8:07 PM on September 5, 2012:

When making my pulls obsolete, which makes sense here to have 1 instead of 2 to fix this, could you please merge the WSAGetLastError() into the printf :)?

jgarzik commented at 8:11 PM on September 5, 2012:

It is nicer this way, especially if you are stepping through with a debugger.
Diapolo commented at 8:15 PM on September 5, 2012: none

Verified to work and fix the 10022 spam on Windows, tried it and closed #1772 in favor of your patch.
laanwj commented at 5:06 AM on September 6, 2012: member

ACK
laanwj referenced this in commit f106491fa2 on Sep 7, 2012
laanwj merged this on Sep 7, 2012
laanwj closed this on Sep 7, 2012
BitcoinPullTester commented at 7:20 AM on September 7, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/2387944782fa61a3137afda91e9e8105d8cc5ddf for binaries and test log.
jgarzik deleted the branch on Aug 24, 2014
KolbyML referenced this in commit ea5f85e071 on Dec 5, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#1772 fix "socket select error 10022" spam