Reconnect after a SOCKS proxy connection failure - #416
Open
Ian-Costa18 wants to merge 1 commit into
Open
Conversation
On the SOCKS path connect() sets this.socket = null and hands the
createConnection() rejection to onSocketError(), whose emit('error') is
commented out. Socket handlers are bound in _onSocketCreate(), which only
runs from the .then, so a rejection has nothing bound to it: no 'close' is
emitted, Connection.socketClose() never runs, and no reconnect is ever
scheduled. The connection sits in SOCK_CONNECTING with a null socket and no
timer, indefinitely.
The direct path assigns this.socket synchronously and calls
_onSocketCreate() immediately, so the same class of failure emits 'close'
and reconnects normally.
Emit the close ourselves when there is no socket to emit it for us. The
!this.socket guard leaves failures arriving through the bound 'error'
handler alone, since those have a real socket that will emit its own
'close'.
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.
The problem
With
options.socksset, a failed connection attempt leaves the connection permanently dead: nocloseis emitted, no reconnect is scheduled, and no error reaches the consumer. The connectionsits in
SOCK_CONNECTINGwith a null socket and no pending timer, indefinitely. The direct(non-proxy) path recovers from the same class of failure normally.
Why
In
connect(), the SOCKS branch setsthis.socket = nulland attaches the rejection handler tothe promise:
_onSocketCreate()is what binds the socket handlers, and it only runs in the.then. So onrejection there is no socket and nothing bound to it. The rejection lands in:
which records the error and emits nothing — the
emit('error')is commented out.Connection.socketClose()is the only place that ever schedules a reconnect, and it is drivenexclusively by the transport's
'close'event, so it never runs.The non-SOCKS branch assigns
this.socketsynchronously and calls_onSocketCreate()immediately, so a failure there emits
'close'and reconnects. The two paths disagree onbehaviour for the same class of failure.
The fix
Emit the close ourselves when there is no socket to emit it for us.
onSocketClose()setsstate = SOCK_DISCONNECTEDand emits'close'withlast_socket_error— exactly what thedirect path produces — after which the existing logic in
Connection.socketClose()appliesunchanged.
The
!this.socketguard keeps failures that arrive through the bound'error'handler frombeing double-counted; those have a real socket that will emit its own
'close'.this.socketisassigned before
_onSocketCreate()in both the plain-SOCKS and TLS-over-SOCKS cases, so a failedTLS handshake correctly takes the bound path.
Tests
Two cases added to the existing
onSocketError()block intest/net.transport.test.js— that aclose is emitted when there is no socket, and that one is not emitted when a socket exists.
Reverting the source change alone turns the first from 248 passing into 247 passing and 1
failing.
yarn lintandyarn unit-testboth clean.Known gap
If the
.thenthrows afterthis.socket = connectionbut before_onSocketCreate()finishesbinding,
!this.socketis false and nothing is bound — the same black hole. Testingthis.socket_events.length === 0would close it precisely. I kept the change minimal instead;say the word if you would rather have the broader guard.
How it was found
A bouncer running four IRC networks through a SOCKS5 proxy. Networks dropped and were never
retried — confirmed against the proxy's own per-connection log, which showed no further
connection attempts for over eight hours. The failure is silent from the consumer's side: no
error event, no
close, nothing in the logs. Restarting the client was the only recovery.