Disable SO_LINGER: a blocking close() freezes the whole socket layer for 5 seconds - #792
Conversation
|
Some archaeology on why the linger is there in the first place, since that seemed worth answering before proposing to remove it. Short version: it looks vestigial.
So the linger is the surviving half of a 2018 pair whose other half was removed eleven weeks later. Today That's not by itself an argument for removing it (a lingering close and a If there is a reason it's still needed that isn't captured in the history, I'd genuinely like to know — I'd rather adjust the patch than remove something load-bearing. The alternative I mentioned in the description (moving I'm also collecting results across a range of kernels to check this isn't version-specific, and will post those here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #792 +/- ##
==========================================
+ Coverage 73.83% 73.87% +0.04%
==========================================
Files 97 97
Lines 8900 8900
Branches 5855 5855
==========================================
+ Hits 6571 6575 +4
- Misses 1720 1721 +1
+ Partials 609 604 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Kernel matrixRan the repro from the description across five kernels to check this isn't version-specific. Each is a separate Lima VM on Apple Virtualization.framework, so each boots its own kernel (containers would have shared the host's, which is why this needed VMs). Median of 3 trials per arm.
Consistent across all five: ~5.1–5.4s blocked with the current setting, 0 ms with the patch, and the That last point is worth stating plainly because it rules out a tempting explanation: this is not a recent kernel regression. Spanning roughly four years of kernel releases, the behaviour is identical. If it seems to have started mattering recently, the change is in workloads that close TCP connections with unacked data more often, not in the kernel. Caveats
|
UnixSocketHandler::write is a partial-write loop that accumulates bytesWritten across iterations, but its five-second EAGAIN give-up never consulted it. A write that had already put a prefix of the buffer on the wire could therefore be abandoned, returning -1 to the caller. That -1 is ambiguous: it cannot be told apart from "nothing was sent", so the caller has no way to know how much of the buffer the peer actually holds. send() returns a short count whenever the socket has room for some but not all of the buffer, so this is an ordinary state under backpressure rather than a rare race. Reproduced standalone on Linux 6.16.1 and macOS 25.6.0: both return -1 with 0 < committed < count (31250/320000 and 11572/320000). ET's callers already survive the ambiguity, and the stream is not corrupted: BackedWriter maps -1 to WROTE_WITH_FAILURE, and Connection turns that into closeSocketAndMaybeReconnect(), replaying unacknowledged messages from the backup buffer keyed by sequence number. The truncated bytes die with the socket. What it costs instead is a reconnect. The give-up path leaves errno as EAGAIN, which isSkippableError() accepts, so ordinary backpressure lands in the "connection is severed" branch and tears the connection down at VLOG(1) -- silently. The fix is therefore about not forcing a spurious reconnect, not about stream integrity. Apply the five-second budget only while nothing has been committed, where -1 is unambiguous and giving up is cheap. Once a byte is on the wire the budget becomes SOCKET_WRITE_COMMITTED_TIMEOUT (60s), so the write is allowed to finish rather than force a teardown. If that ceiling is reached, log at ERROR with the committed count and the total, so the ambiguous case is never silent. A real error from send() still returns immediately through the existing else branch. Adds three cases to test/unit_tests/UnixSocketHandlerTest.cpp: WriteCompletesOnSlowlyDrainingSocket (fails on the unpatched code), WriteStillGivesUpWhenNothingWasSent, and WriteFailsFastOnClosedPeer. Follow-up to MisterTea#792.
|
@marchaase Please get CI to be green, thanks |
|
@copilot rebase this on latest master |
…for 5 seconds
Every TCP socket etserver accepts gets SO_LINGER {l_onoff=1, l_linger=5} in
TcpSocketHandler::initSocket. When one is closed with data still unacknowledged,
close() blocks for the full five seconds.
What makes it bite is where the close happens: UnixSocketHandler::close() takes
the process-wide globalMutex (line 170), forces the fd blocking at 183 (sockets
are created non-blocking, and Linux ignores a nonzero linger on a non-blocking
socket, so that line is what arms it), then calls the blocking ::close() at 190.
The lock_guard is function-scoped, so the mutex is held until 194.
So it is not one listener stalling: every thread in etserver that touches any
socket stalls for five seconds because one unrelated connection is tearing down.
Interactive terminal sessions are among those threads.
Setting l_onoff = 0 disables the linger without removing the setsockopt call.
Measured with a standalone repro containing no ET code (peer accepts and never
reads, fill the send buffer, set SO_LINGER, close):
l_onoff=1, l_linger=5 close() 5156 5118 5120 5119 5118 5120 ms (18/18 blocked)
no SO_LINGER close() 0 0 0 0 0 0 ms (18/18 instant)
l_onoff=0, l_linger=0 close() 0 0 0 0 0 0 ms
l_onoff=1, l_linger=2 median 2046.5 ms
Halving l_linger halves the stall (ratio 0.3998 vs a predicted 0.4000), which
rules out the other two five-second constants in the codebase.
A patched etserver vs stock, 20 trials with alternating arms and an identical
1,824,768 bytes queued every trial:
l_onoff=1 (stock) n=10 median 5.1184s max 5.4877s 10/10 blocked
l_onoff=0 (patched) n=10 median 0.0000s max 0.0000s 0/10
Note: this will not reproduce on macOS, which drains the receive queue during
close() even when the peer never reads (~51ms rather than ~5000ms).
330c473 to
1689d26
Compare
|
Thank you! |
2 participants
The bug
Every TCP socket
etserveraccepts getsSO_LINGER {l_onoff=1, l_linger=5}inTcpSocketHandler::initSocket. Closing such a socket while it still has unacknowledged data blocks in::close()for the full five seconds.That alone would only slow one teardown. The problem is where the close happens —
UnixSocketHandler::close():globalMutex(line 170),setBlocking(fd, true)) — sockets are created non-blocking, and Linux ignores a nonzero linger on a non-blocking socket, so this line is what arms it,::close()at line 190,lock_guardis function-scoped, so the mutex is held until 194.So it isn't one connection stalling. Every thread in
etserverthat touches any socket stalls for five seconds because one unrelated connection is tearing down, including the threads serving interactive terminal sessions.Only about half of teardowns stall, since the linger arms only when there is unacknowledged data in the send queue at close time. That intermittency is likely why it has gone unnoticed — it looks like a network problem.
Why the linger is there
Worth answering before proposing to remove it. From the history, it looks vestigial.
410aa69b8(PR #127, "Fix reconnect issue with jumphost")SO_LINGERfirst appears — one line inside a multi-feature PR. The PR body does not mention it. The commit's own bullets are the only context: "Call shutdown upon closing a connection" and "Kill socket & reconnect less agressively".01d8c9298TcpSocketHandlerduring the socket refactor, unchanged.a733381f3("remove shutdown")::shutdown(fd, SHUT_RDWR)deleted. Empty commit body, no stated reason.1479bdce9So
shutdown()andSO_LINGERwent in together as a teardown pair, in service of jumphost reconnect stability, and theshutdown()was removed eleven weeks later. The linger is the surviving half. Todayclose()issetBlocking(fd, true)then::close(fd), withSO_LINGERarmed and noshutdown()— not the configuration either commit designed.That is not on its own an argument for removal (a lingering close and a
shutdown()do different things), but it does mean no documented behaviour depends on it, and1479bdce9is precedent for narrowing its scope when it caused problems.If there is a reason it is still needed that the history does not capture, I would rather hear it and adjust the patch than remove something load-bearing.
Reproducing it
Self-contained, no ET involved. A peer accepts and never reads, so the sender's data stays unacknowledged; then close a socket that has
SO_LINGERset.The load-bearing detail is that the peer must never call
recv(). Without unacknowledged data in the send queue at close time,SO_LINGERnever arms and every variant returns instantly.The fix
Set
l_onoff = 0. Thesetsockoptcall is left in place.Testing
The script above, on Linux:
The
l_linger=2arm is the control: halving the constant halves the stall, ratio 0.3998 against a predicted 0.4000. That matters because there are two other five-second constants nearby —UnixSocketHandler.cpp:43 waitForData(fd, 5, 0), and thepselect6 tv_sec=5that is simply that call as it appears in a trace — and this rules both out by measurement rather than by argument.Then a patched
etserveragainst stock, 20 trials, arms alternating, identical 1,824,768 bytes queued in every trial:Confirmed at the syscall level on the running service —
setsockopt(fd, SOL_SOCKET, SO_LINGER, {l_onoff=0, l_linger=5}, 8) = 0, versus{l_onoff=1, l_linger=5}for a stock binary under the same test.Notes
This will not reproduce on macOS. The Mac drains the receive queue during
close()even when the peer never reads, so you get ~50ms instead of ~5000ms.After the change, a burst of simultaneous teardowns leaves sockets in
LAST-ACKwithSend-Q=1(the unacked FIN) for a while. That is not a leak —ss -tanpshows an empty process column, so they are kernel orphans already closed, bounded bynet.ipv4.tcp_max_orphans. It is the expected consequence of teardowns no longer being serialized at ~5s each.An alternative fix, if the linger is wanted, is moving
::close()outsideglobalMutex— a blocking close inside a process-wide lock is questionable regardless of the timeout. Either one alone fixes the freeze; this PR takes the smaller change, and I am happy to do the other instead if you prefer.This code is byte-identical in 6.2.11 and 7.0.0, so it is long-standing rather than a recent regression.