Repository navigation
fix(engine.io-client): send the close packet when closing mid-handshake - #5575
Open
Jeremiah-D wants to merge 1 commit into
Open
Jeremiah-D wants to merge 1 commit into
Jeremiah-D wants to merge 1 commit into
Conversation
If close() is called on a polling transport while the handshake is still in flight, doClose() defers writing the close packet until the transport opens, but Transport#close() then calls onClose() synchronously, flipping the transport to "closed" so the deferred write never happens. Defer onClose() along with the close packet instead, and drive the deferred write off the handshake completion in onData rather than a one-off "open" listener (the Socket clears all transport listeners when it closes, which would silently drop that listener). The sid is read from the open packet itself, so the close packet stays routable even when the Socket has already dropped its listeners.
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.
Fixes #5274 (bug 2).
Hey — I dug into the second bug from #5274 on current master and could reproduce it: closing a polling transport mid-handshake never sends the close packet.
Transport.close()callsonClose()synchronously right afterdoClose(), so the deferredonce("open", ...)inPolling.doClose()can never fire — by the time the handshake response arrives the transport is alreadyclosedand the open packet is ignored.Two changes:
Transport.close()now skips the synchronousonClose()whendoClose()deferred the close (newcloseDeferredflag); the transport callsonClose()itself once the deferred close runs.Polling.doClose()no longer relies on a one-off"open"listener for the deferred close —Socket._onClose()clears all transport listeners on close, which would silently drop it. Instead the deferred close is picked up inonDatawhen the handshake's open packet arrives, and the sid is read from the open packet itself since the Socket may already have dropped its listeners.I also checked bug 1 from the issue while I was there: it does not reproduce on the JS side.
flush()bails whentransport.writableis false, and both the polling and websocketwrite()flip that flag synchronously, so back-to-backsendPacketcalls just queue up until the next drain. Looks specific to the Java port's asyncEventThreaddispatch.Verified with two new tests in
test/transport.js("close during handshake"): stubbed fetch, handshake GET held back, close mid-handshake, then release. Both fail without the fix (no close packet POSTed) and pass with it — exactly one1packet POSTed with the sid in the URL, and no further polling afterwards. Fulltest/transport.js: 25 passing; the only 2 failures are pre-existingperMessageDeflatetimeouts that fail identically on unmodified master in this environment.