net: ~SocketBackend takes _socketMtx across shutdownBoth(), the deadlock SocketServer::close() documents avoiding

[Yaraslaut]

Split out of #498, which bundled it with an unrelated resource leak. Filed separately because the fix, the file and the severity are all different.

Summary

~SocketBackend acquires _socketMtx before calling shutdownBoth(). sendFrame holds that same mutex across a blocking, un-timed sendAll. Against a stalled peer the destructor blocks on the very lock whose holder only shutdownBoth() would release.

Verification status

Inferred from reading the code; not reproduced. I did not construct a stalled-peer teardown. Revision: origin/master adfe8e5f plus doc-only commits.

socket_backend.hpp:89-93:

std::scoped_lock lock{_socketMtx};
if (_socket.valid()) { _socket.shutdownBoth(); }

sendFrame (:380-387) takes _socketMtx and then calls _socket.sendAll(frame.data(), frame.size()); sendAll (tcp_socket.hpp:327-339) loops on a blocking ::send with no timeout.

Why this is a defect and not a design choice

The sibling class states the trap and deliberately does the opposite. SocketServer::close() (socket_server.hpp:174-183):

"Taking it here would deadlock exactly when the shutdown is most needed: a client thread blocked in sendAll against a stalled peer's full socket buffer holds writeMtx for as long as that send is stuck, and close() has no timeout — it is reached from the destructor. shutdownBoth() is documented safe from any thread and is itself the mechanism that unblocks that send … Locking to 'protect' the socket would therefore wait on the very thing it is trying to interrupt."

Every clause applies to ~SocketBackend. shutdownBoth() is documented safe from any thread (tcp_socket.hpp:341-342), so the lock buys nothing that the I/O thread's ownership of _socket's writes does not already give.

The I/O thread's own Pong/Close echo in drainFrames (socket_backend.hpp:583, :589) reaches sendFrame too, so the holder need not even be an application thread.

What would change the verdict

• Close it if sendFrame is shown unreachable concurrently with ~SocketBackend.
• Raise it by reproducing: a peer that accepts and never reads, filling the send buffer, then destroy the backend.

A regression test must actually stall the peer — a test that destroys an idle backend would pass with or without the fix.

[Yaraslaut]

The obvious fix is wrong — ThreadSanitizer says so. Staying open.

I implemented this (drop _socketMtx from ~SocketBackend, on the grounds that shutdownBoth() is documented safe from any thread) and CI's clang-tsan leg failed on "SocketBackend: action result delivered via then":

#0 TcpSocket::closeNow()             tcp_socket.hpp:434
#1 TcpSocket::operator=(TcpSocket&&) tcp_socket.hpp:82
#2 SocketBackend::onDisconnected()   socket_backend.hpp:494
      vs
#0 TcpSocket::valid()                tcp_socket.hpp:395
#1 SocketBackend::~SocketBackend()   socket_backend.hpp:100

Where the reasoning failed. shutdownBoth() being thread-safe is true and irrelevant: _socketMtx is not only guarding use of the socket at this site, it guards _socket against replacement. onDisconnected() does _socket = TcpSocket{} — a move-assign that closes the old fd — so an unlocked _socket.valid() in the destructor races the I/O thread swapping the object out from under it.

The SocketServer::close() precedent this issue and I both leaned on does not have that shape: it shuts down sockets it never reassigns. That difference is the whole thing, and neither the issue nor I noticed it.

Measured both ways locally rather than trusting CI: 25 race reports without the lock, 0 with it.

What still stands

The hazard as originally described is real and unchanged: sendFrame holds _socketMtx across a blocking, un-timed sendAll, so a peer that accepts and stops reading can park ~SocketBackend on that lock, and only shutdownBoth() would clear it.

What a real fix needs

Not a dropped lock — a way to reach the fd without the mutex. The shape that works is an std::atomic<int> shadowing _socket's descriptor, updated wherever _socket is assigned, with the destructor calling ::shutdown() on that value directly. That carries its own fd-reuse hazard (the fd could be closed and recycled between the load and the shutdown), which needs designing rather than asserting.

Both the code comment and this issue now record why the simple version is not available, so the next person does not spend the same CI cycle discovering it.

Re-labelling stays triage: valid; the premise was never in doubt, only the fix.