net: one dead descriptor must not cost every peer its turn

From the first of two wedges captured on the pool. It is a different
proximate fault from the second one, and this is the part that turns a
single bad socket into a node-wide outage.

A failed select() zeroes both descriptor sets, so no peer is serviced on
that pass. That is correct as far as it goes. What was missing is that
nothing stopped the offending descriptor coming straight back:

  - the invalid socket was only marked fDisconnect, and a node is not
    removed from vNodes until it has drained or DISCONNECT_DRAIN_SECS has
    elapsed -- and it cannot drain, because its socket is gone;
  - the select set was rebuilt from vNodes with no check, so the same dead
    descriptor went back in on the very next pass and failed select()
    again.

Measured in the capture: 1,027 consecutive 'its socket is no longer valid'
passes across 61 seconds -- the full drain timeout -- one bad socket, and
for the whole of it the node read nothing from anybody. The peer that
caused it had 57 bytes unsent, so it took the slow path out.

Now the descriptor is forgotten as soon as getsockopt says it is not ours:
select() succeeds again on the next pass and every other peer is served
normally while the dead node ages out as before.

Deliberately forgotten rather than closed. getsockopt has just said this
descriptor is no longer ours; calling close() on it is precisely how an
unrelated socket gets closed instead, which is the fault fixed in e742d0b.

(cherry picked from commit f14c513b4e39d6203b73584f6dc01acbb5a9546e)

1 files changed, 23 insertions, 1 deletions

diff --git a/src/net.cpp b/src/net.cpp
index 964097b..77f9acf 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1471,11 +1471,29 @@ void ThreadSocketHandler2(void* parg)
 #else
                     socklen_t nTypeLen = sizeof(nType);
 #endif
-                    if (getsockopt(pnode->hSocket, SOL_SOCKET, SO_TYPE, (char*)&nType, &nTypeLen) != 0)
+                    if (pnode->hSocket != INVALID_SOCKET &&
+                        getsockopt(pnode->hSocket, SOL_SOCKET, SO_TYPE, (char*)&nType, &nTypeLen) != 0)
                     {
                         printf("dropping node %s: its socket is no longer valid\n",
                                pnode->addr.ToString().c_str());
                         pnode->fDisconnect = true;
+
+                        // Stop referring to it, or it goes back into the next
+                        // select() set and fails it again.
+                        //
+                        // Marking fDisconnect is not enough: a node is only
+                        // removed from vNodes once it has drained or once
+                        // DISCONNECT_DRAIN_SECS has passed, so one dead
+                        // descriptor kept failing select() for a full minute --
+                        // and a failed select() zeroes both sets above, which
+                        // means NO peer is read on that pass. Measured on a
+                        // wedged pool: 1,027 consecutive passes over 61
+                        // seconds, one bad socket, the entire node deaf.
+                        //
+                        // Not closed, only forgotten. getsockopt just told us
+                        // this descriptor is not ours any more; closing it now
+                        // is how an unrelated socket gets closed instead.
+                        pnode->hSocket = INVALID_SOCKET;
                     }
                 }
             }
@@ -1548,6 +1566,10 @@ void ThreadSocketHandler2(void* parg)
         {
             CheckForShutdown(0);
             SOCKET hSocket = pnode->hSocket;
+            // Forgotten above because it was already invalid. FD_ISSET on it
+            // is meaningless and recv()/send() would be operating on -1.
+            if (hSocket == INVALID_SOCKET)
+                continue;
 
             //
             // Receive