diff --git a/src/btfrv.cpp b/src/btfrv.cpp index 63e092e..7efd38b 100644 --- a/src/btfrv.cpp +++ b/src/btfrv.cpp @@ -258,10 +258,57 @@ RvSocket RvServiceRegister(const char* relay_host, unsigned short port, return (RvSocket)s; } -bool RvServiceWaitPaired(RvSocket s) +bool RvServiceWaitPaired(RvSocket s, int nTimeoutSecs) { + // A registration socket is idle on purpose -- we announce ourselves and + // wait, possibly for a long time, for someone to dial. ConnectTo() clears + // the socket timeout for exactly that reason, and with no timeout at all + // this recv() has no way to end when the path dies without either side + // saying so: a NAT drops the idle mapping, a relay restarts, a route + // changes. No FIN arrives, nothing is readable, and the call never + // returns. The thread stays alive and asleep, so ThreadBtfAccept never + // reaches the `continue` that would register again, and the node goes deaf + // while every outward sign says it is healthy. + // + // Seen in production on the bootstrap seed: 27 minutes without a block, + // eight behind the network, relay reachable throughout, fixed instantly by + // a restart. + // + // TCP keepalive is the obvious answer and it does not work here -- I + // measured it. With probes every 5s and a 2-probe limit, a socket whose + // replies were being dropped was still ESTAB after four minutes, the probe + // counter stuck at zero. So the timeout is ours to enforce, at the one + // place that knows what waiting means. + // + // Expiring is not a failure. It costs one reconnect per interval and + // returns the loop to a known state. + if (nTimeoutSecs > 0) + { +#ifdef _WIN32 + DWORD tv = (DWORD)nTimeoutSecs * 1000; + setsockopt((SOCKET)s, SOL_SOCKET, SO_RCVTIMEO, (const char*)&tv, sizeof(tv)); +#else + struct timeval tv; tv.tv_sec = nTimeoutSecs; tv.tv_usec = 0; + setsockopt((SOCKET)s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)); +#endif + } + unsigned char paired = 0; - return ReadN((SOCKET)s, &paired, 1) && paired == 0x01; + bool fOk = ReadN((SOCKET)s, &paired, 1) && paired == 0x01; + + // Hand back a socket that blocks again: from here it is a data tunnel, and + // a tunnel legitimately sits quiet between messages. + if (fOk && nTimeoutSecs > 0) + { +#ifdef _WIN32 + DWORD tvOff = 0; + setsockopt((SOCKET)s, SOL_SOCKET, SO_RCVTIMEO, (const char*)&tvOff, sizeof(tvOff)); +#else + struct timeval tvOff; tvOff.tv_sec = 0; tvOff.tv_usec = 0; + setsockopt((SOCKET)s, SOL_SOCKET, SO_RCVTIMEO, &tvOff, sizeof(tvOff)); +#endif + } + return fOk; } RvSocket RvClientConnect(const char* relay_host, unsigned short port, diff --git a/src/btfrv.h b/src/btfrv.h index 9d6756e..9cd7476 100644 --- a/src/btfrv.h +++ b/src/btfrv.h @@ -49,7 +49,12 @@ RvSocket RvServiceRegister(const char* relay_host, unsigned short port, // Block on a registered service socket until a client is paired to us. Returns // false if the relay drops us or errors, in which case the caller should close // the socket and register again. -bool RvServiceWaitPaired(RvSocket s); +// Blocks until a client is paired with us. Give it a timeout, in seconds, and +// it returns false once that passes with nobody arriving -- which the caller +// should treat as "register again", not as an error. Zero means wait forever, +// which is what this used to do unconditionally and must not be used for a +// registration socket: see the note on the implementation. +bool RvServiceWaitPaired(RvSocket s, int nTimeoutSecs = 0); // ---- client side (the node dialing a .btf address) ---- // Connect through the relay to the service registered under `target_pubkey`. diff --git a/src/net.cpp b/src/net.cpp index 395092d..2581578 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -783,12 +783,17 @@ void ThreadBtfAccept(void* parg) LogPrint("net", "rendezvous: registered at %s, waiting for a dial\n", strMeeting.c_str()); - // Now block for someone to arrive. - if (!btf::RvServiceWaitPaired(rv)) + // Now wait for someone to arrive -- but not forever. This used to have + // no bound, and a registration whose path died quietly left the thread + // parked in recv() with nothing to wake it: the loop never came back + // here, the node stopped being reachable, and nothing in the log said + // so. Re-registering every few minutes when nobody has dialled costs + // one reconnect and removes the whole failure mode. + if (!btf::RvServiceWaitPaired(rv, BTF_RENDEZVOUS_WAIT_SECS)) { btf::RvClose(rv); - LogPrint("net", "rendezvous: %s dropped us before any dial, re-registering\n", - strMeeting.c_str()); + LogPrint("net", "rendezvous: no dial at %s within %ds (or it dropped us), " + "re-registering\n", strMeeting.c_str(), BTF_RENDEZVOUS_WAIT_SECS); continue; } diff --git a/src/net.h b/src/net.h index 364fbfb..4379d04 100644 --- a/src/net.h +++ b/src/net.h @@ -43,6 +43,14 @@ static const unsigned int MAX_PEX_DESCRIPTOR_BYTES = 1024; // connection is the intended traffic; anything faster is someone else's idea. static const int64 PEX_MIN_INTERVAL = 60; +// How long to sit registered at a rendezvous relay before giving up on this +// attempt and registering again. Bounded because a registration socket that +// goes quiet is indistinguishable, from inside recv(), from one whose path has +// died -- and the second kind never wakes up. Five minutes is short enough +// that a node is unreachable only briefly and long enough that the reconnect +// traffic is nothing: one per relay per five minutes. +static const int BTF_RENDEZVOUS_WAIT_SECS = 300; + // Descriptors to hand a peer: ours first, then peers that answered us. void BtfPexCollect(std::vector& vDescOut); // Verify descriptors a peer sent and remember the good ones. Returns how many.