Repository navigation
fix(initiator): bound writes on stop and close the connection - #798
Open
joshkeldam wants to merge 2 commits into
Open
joshkeldam wants to merge 2 commits into
joshkeldam wants to merge 2 commits into
Conversation
A socket write has no deadline, the session loop waits for the writer to take each message, and Initiator.Stop waits for the session loop. When the counterparty stops reading, Stop either hangs (the writer was already blocked, so the session loop cannot take the stop) or returns while the connection is still open (the writer is blocked on the Logout itself, after the session timed the Logout out). Only the writer closes the connection, so nothing else ever does. Once the initiator stops, every write, including one already in progress, now has LogoutTimeout to finish, set again for each write. A write that misses it closes the connection, which frees the writer and the session loop. After the session has stopped, the writer gets LogoutTimeout to finish its last message, so a graceful Logout is not cut short, and then the transport under any TLS is closed, so Stop never returns with the connection open. Tests cover a peer that stops reading with the socket buffers full, over TCP and TLS (Stop hangs without this change), a peer that answers the Logout (still graceful), and a write blocked on the Logout itself.
The TLS handshake took no context and had no deadline, although SocketTimeout is documented as its timeout: v0.9.10 applies it to the TCP dial only. A stop during a stalled handshake therefore waited until the kernel gave up on the connection, and a counterparty that accepted the connection and never answered held the attempt forever, with no reconnect. A failed handshake, or a failed session start, also went back to reconnect without closing the connection it had dialed, leaking one socket per attempt. The handshake now runs under the dial context, so Stop cancels it, within SocketTimeout, or LogonTimeout when SocketTimeout is unset. Both failure paths close the transport before reconnecting. loadSocketTimeout reads the setting for the dialer and the handshake alike. Tests: Stop during a handshake the peer never answers, a stalled handshake that times out and reconnects, a failed handshake that closes its connection, and a slow handshake inside SocketTimeout that is not cut short by LogonTimeout.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Initiator.Stopcan hang, or return while the connection is still open, when the counterparty stops reading. A socket write has no deadline, the session loop waits for the writer to take each message over an unbuffered channel, andStopwaits for the session loop. If the writer is already blocked whenStopis called, the session loop is stuck on its next send, never takes the stop request, andStophangs until the kernel gives up on the TCP connection (minutes, with Linux defaults). If the writer blocks on the Logout itself, the session loop times the Logout out and exits, soStopreturns, but only the writer ever closes the connection, so it stays open. A caller that treats a returnedStopas "the session is gone" can then log the same session on again beside it, which is how we hit it on a FIX bridge that hands its session between replicas.This change makes the stop path in
handleConnectionown the connection's end. Once stopping, every write, including one already in flight, hasLogoutTimeoutto finish, and the deadline is set again for each write, so a Logout sent after a slow application callback still gets its own window. A write that misses it closes the connection, which unblocks the writer and lets the session loop take the stop. After the session has stopped, the writer getsLogoutTimeoutto finish its last message, so a graceful Logout (or the reply to the counterparty's) is not cut short, and then the transport is closed, the raw connection under any TLS, soStopnever returns with it open. Closing the raw connection matters for TLS: a secondtls.Conn.Closereportsnet.ErrClosedwhile the first is still sending its close notification, with the socket open.The cost is that a counterparty that is still reading, but takes longer than
LogoutTimeoutfor a single write during shutdown, now gets disconnected instead of waited on indefinitely. A clean shutdown with a responsive counterparty is unchanged, and the acceptor path is not touched.The new tests in
initiator_test.godrivehandleConnectionagainst a real acceptor: one through a peer that stops reading with the socket buffers full, over TCP and TLS (it hangs inStopwithout this change), one with a peer that answers the Logout (still graceful, over TCP and TLS), and one where the write blocks on the Logout itself.The second commit closes the same kind of gap on the way in. The TLS handshake took no context and no deadline, although
SocketTimeoutis documented as its timeout and is only applied to the TCP dial. A stop during a handshake the counterparty never answers waited on the kernel, and without a stop the attempt held forever with no reconnect. The handshake now runs under the dial context, soStopcancels it, withinSocketTimeout, orLogonTimeoutwhen that is unset (the setting's doc now says so). A failed handshake, or a failed session start on a new connection, now closes the socket it dialed instead of leaking it on every attempt.