Repository navigation
Fix graceful shutdown hanging and killing in-flight requests - #713
Open
kksingh000 wants to merge 1 commit into
Open
kksingh000 wants to merge 1 commit into
kksingh000 wants to merge 1 commit into
Conversation
When an Application was listening with an AbortSignal and that signal was aborted while requests were in flight, two problems occurred: 1. listen() never resolved. Server#listen() in http_server_native.ts never closed the ReadableStream's controller when the underlying Deno.serve() instance shut down, so the `for await` loop consuming that stream in Application#listen() hung forever even after all in-flight requests had been handled. 2. In-flight requests were killed instead of completing. Server#listen() passed `signal` directly into Deno.serve()'s own options, which causes Deno's engine to hard-abort in-flight connections immediately on signal abort. It also registered its own `abort` listener that called `this.close()` (and therefore `httpServer.shutdown()`) straight away, racing Application#listen()'s own careful tracking of in-flight requests (`state.handling`), which already defers calling `Server#close()` until those requests have drained. Fix both by: - Closing the stream's controller once Deno.serve()'s own `.finished` promise resolves, so the consuming `for await` loop (and therefore the top-level `listen()` promise) terminates correctly. - No longer passing `signal` into Deno.serve()'s own options, and removing Server#listen()'s own abort-triggered call to `close()`. Application#listen() already owns the correct abort-then-drain- then-close sequencing; Server no longer races it. Fixes oakserver#611 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126cmS4qpPbYhiSmnHo3cUo
This branch has not been deployed
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.
When an Application was listening with an AbortSignal and that signal was aborted while requests were in flight, two problems occurred:
listen() never resolved. Server#listen() in http_server_native.ts never closed the ReadableStream's controller when the underlying Deno.serve() instance shut down, so the
for awaitloop consuming that stream in Application#listen() hung forever even after all in-flight requests had been handled.In-flight requests were killed instead of completing. Server#listen() passed
signaldirectly into Deno.serve()'s own options, which causes Deno's engine to hard-abort in-flight connections immediately on signal abort. It also registered its ownabortlistener that calledthis.close()(and thereforehttpServer.shutdown()) straight away, racing Application#listen()'s own careful tracking of in-flight requests (state.handling), which already defers callingServer#close()until those requests have drained.Fix both by:
.finishedpromise resolves, so the consumingfor awaitloop (and therefore the top-levellisten()promise) terminates correctly.signalinto Deno.serve()'s own options, and removing Server#listen()'s own abort-triggered call toclose(). Application#listen() already owns the correct abort-then-drain- then-close sequencing; Server no longer races it.Fixes #611
Claude-Session: https://claude.ai/code/session_0126cmS4qpPbYhiSmnHo3cUo