fix: gracefully handle sends on closed server during shutdown#70
Closed
shields wants to merge 5 commits into
Closed
fix: gracefully handle sends on closed server during shutdown#70shields wants to merge 5 commits into
shields wants to merge 5 commits into
Conversation
MDNSServer.sendResponse, sendOnAllNetworksForService, and send now gracefully handle the closed state instead of throwing ServerClosedError. Responder.shutdown cancels pending delayed multicast response timers via new QueuedResponse.cancel method. Remove now-unused ServerClosedError class and ERR_SERVER_CLOSED constant (neither was part of the public API).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…shutdown-race-condition
Coverage Report for CI Build 26105798958Coverage remained the same at 36.493%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Contributor
Author
|
No diff because 5c80603 was merged into |
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.
(This is a duplicate of PR #60, which was closed accidentally.)
MDNSServer.sendResponse, sendOnAllNetworksForService, and send now gracefully handle the closed state instead of throwing ServerClosedError. Responder.shutdown cancels pending delayed multicast response timers via new QueuedResponse.cancel method. Remove now-unused ServerClosedError class and ERR_SERVER_CLOSED constant (neither was part of the public API).
♻️ Current situation
Responder.shutdown() calls currentProber.cancel() which calls clearTimeout(), but if the timer callback is already dequeued, clearTimeout can't prevent it from executing. The callback then calls MDNSServer.sendQueryBroadcast(), which
synchronously calls assertBeforeSend() and throws ServerClosedError. Since sendProbeRequest runs inside setTimeout callbacks with
no try/catch, this becomes an uncaught exception.
The same race exists for delayed multicast responses in the Responder: shutdown() closes the server but never cancels the
QueuedResponse timers, so their callbacks can fire after the server is closed and throw ServerClosedError from sendResponse.
The Responder had a try/catch for the delayed response case, but the Prober had no protection at all.
This probably isn't very important in production, since it happens on the shutdown path. However, it was causing spurious test failures in some code I'm working on.
💡 Proposed solution
Fix at two levels:
instead of throwing. This makes the server safe to call during shutdown regardless of timing, and removes the need for callers to
catch ServerClosedError.
prober and broadcast interval cancellation. This prevents unnecessary work after shutdown, not just errors. A new
QueuedResponse.cancel() method supports this.
ServerClosedError and ERR_SERVER_CLOSED are removed; neither were exposed in the API.
⚙️ Release Notes
Fixes an uncaught ERR_SERVER_CLOSED exception that could occur during shutdown when timer
callbacks raced with server teardown.
➕ Additional Information
Testing
Added QueuedResponse.spec.ts with a test that schedules a response callback, cancels it, and verifies the callback never fires.
This directly tests the new cancel() method that the shutdown cleanup relies on.