Skip to content

fix(network): close peer connections that go idle (backport of #4562) - #4568

Open
stevenvegt wants to merge 1 commit into
V5.4from
V5_4/idle-timeout
Open

stevenvegt wants to merge 1 commit into
V5.4from
V5_4/idle-timeout

Conversation

@stevenvegt

Copy link
Copy Markdown
Member

Backport of #4562. Follows the backport of #4467 (#4564 on this branch), which fixed the other half of the same incident.

Problem

A peer kept a stream open for our node after our node had already dropped it, so it believed it was still connected and rejected every new connection with already connected. In our case the stale stream was held by an nginx in front of the peer; a half-open TCP connection does the same. Nothing cleaned that up short of restarting the proxy.

Change

New network.idletimeout (default 2m, 0 disables). Peers send gossip and diagnostics on every stream every few seconds, so a stream with no incoming message for two minutes is dead. The node now closes it and the regular reconnect logic takes over. Time spent handling a message (a large transaction list during sync, for example) does not count as idle. This works through reverse proxies, unlike gRPC keepalive pings, which proxies answer themselves.

The second part of the original PR is a refactor with no behaviour change: the three places that start a goroutine for a connection now go through startGoroutine or startReceiveLoop, so the goroutine counter and the receive-loop WaitGroup are maintained in one place instead of by hand at each site.

Conflicts

  • docs/pages/release_notes.rst: this branch keeps its own Unreleased section. Resolved by adding the idle-timeout entry in this branch's style, below the three entries from fix(network): back off when a peer rejects the connection (backport of #4467) #4564.
  • docs/pages/deployment/server_options_didnuts.rst does not exist on this branch. Dropped it; server_options.rst and cli-reference.rst were regenerated instead and carry the new option.

The code applied without conflicts, including the /v5 module paths, because the changed hunks do not touch import lines.

Verification

go test -race ./network/... passes, apart from TestNetwork_checkHealth, which fails locally for environment reasons on this branch without the change as well. Generated docs are up to date.

Note on TestNetworkIntegration_OutboundConnection11Reconnects: it failed twice during the full-package runs while preparing this backport. It is not caused by this change (6 clean full-package runs with it, 4 without it, 10 clean runs of the test in isolation). The test waits defaultTimeout (5s) for a reconnect while a clean disconnect sets a random 1 to 5 second backoff, so the budget is too tight under load. Pre-existing, same family as #4321.

Operational notes

For deployments behind nginx, grpc_read_timeout and grpc_send_timeout at their 60s default (not raised to hours) plus so_keepalive on the listen directive let the proxy drop dead streams on its own as well.

* fix(network): close peer connections that go idle

Peers send gossip and diagnostics messages every few seconds, so a
stream without any incoming message is a dead one: a half-open TCP
connection, or a reverse proxy that kept the stream open after the
other side went away. Such a connection lingered until the proxy or
node was restarted, and the peer holding the stale stream rejected
every new connection with "already connected".

Connections on which nothing was received for network.idletimeout
(default 2m) are now closed, after which the regular reconnect logic
takes over. Time spent handling a message does not count as idle, so a
large transaction list during sync does not trigger a disconnect. Set
network.idletimeout to 0 to disable the check.

This works through reverse proxies, unlike gRPC keepalive pings, which
proxies answer themselves.

Assisted-by: AI

* refactor(network): start connection goroutines through one helper

A connection keeps two pieces of bookkeeping for its goroutines: an
atomic counter that the tests assert on, and a WaitGroup that
waitForReceivers blocks on. Neither can replace the other. A WaitGroup
can be waited on but its value cannot be read; an atomic counter can be
read but cannot be waited on. Waiting on all goroutines instead of the
receive loops would be wrong as well, because startSending can block in
SendMsg for as long as the gRPC transport takes to give up on a dead
connection.

Both were maintained by hand at each of the three places that start a
goroutine, so adding a receive loop meant remembering two separate
registrations. They now go through startGoroutine and startReceiveLoop,
which do the bookkeeping, and the counter and the WaitGroup are no
longer touched anywhere else. The tests read the counter through
goroutineCount.

No behaviour change.

Assisted-by: AI
(cherry picked from commit 13187b8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants