fix(connect): make timeout cover the STOMP handshake (fix hung-CONNECT socket leak)#138
Open
amilcarsilvestre wants to merge 1 commit into
Conversation
The connect `timeout` only covered the transport (TCP/TLS) connect: it was
cleared in onConnected (on transport connect) before the STOMP
CONNECT/CONNECTED handshake ran, and that handshake had no timeout of its own
(heartbeats are only armed after CONNECTED). A server that accepts the socket
but never sends CONNECTED left the connection hung forever, with the socket
open and the callback never invoked.
Keep the timeout armed until the handshake settles (cleared in a wrapped
connectListener). A stalled handshake now triggers client.destroy('connect
timed out'), closing the socket and surfacing an error to the callback.
timeout now bounds the whole connect (transport + handshake). Adds a
regression test.
Co-Authored-By: Claude <noreply@anthropic.com>
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
1 participant
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.
Make connect
timeoutcover the STOMP handshake (fix hung-CONNECT socket leak)Fixes #137.
Problem
The
timeoutoption ofconnect()only covered the transport (TCP/TLS) connect.onConnected()(fired on transport connect) calledcleanup()— whichclearTimeout(timeout)— before running the STOMPCONNECT→CONNECTEDhandshake. So a server that accepts the socket but never returnsCONNECTEDleft the connection hung with no timeout and the socket open indefinitely (the callback never fires, and heartbeats are only armed afterCONNECTED).Change
In
lib/connect.js,onConnected()no longer clears the timeout. It removes only the transport-level listeners and keeps the timeout armed whileclient.connect(...)performs the STOMP handshake; the timeout is cleared when the handshake completes or errors (via a wrapper aroundconnectListener). If the handshake stalls, the existing timeout handler firesclient.destroy('connect timed out'), which closes the socket and surfaces the error to the callback.Behavior change:
timeoutnow bounds the whole connect (transport + STOMP handshake), which matches what a "connect timeout" is expected to mean. No API change.Test
Added a test: a fake transport that connects but never sends
CONNECTEDmust causeconnect()to error withintimeoutand destroy the socket (no leak).