Stop the GraphQL WebSocket deadline scheduler when the servlet is destroyed - #858
Open
sergehuber wants to merge 2 commits into
Open
Stop the GraphQL WebSocket deadline scheduler when the servlet is destroyed#858sergehuber wants to merge 2 commits into
sergehuber wants to merge 2 commits into
Conversation
…troyed SubscriptionWebSocketFactory is only ever handed to Jetty as the socket creator; it is never started or stopped as a lifecycle, so the doStop() override that was meant to shut its scheduler down never ran and the thread outlived each servlet. Shut it down explicitly from GraphQLServlet.destroy() instead, and cover it with a test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The regression test does not verify that servlet destruction invokes scheduler shutdown.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Explicitly stops the GraphQL WebSocket authentication deadline scheduler during servlet destruction.
Changes:
- Adds servlet-driven scheduler shutdown.
- Replaces the unused lifecycle hook with
shutdown(). - Adds scheduler shutdown unit coverage.
File summaries
| File | Description |
|---|---|
GraphQLServlet.java |
Shuts down the socket creator during destruction. |
SubscriptionWebSocketFactory.java |
Exposes explicit scheduler shutdown. |
SubscriptionWebSocketFactoryTest.java |
Tests scheduler shutdown state. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The factory test only showed that shutdown() stops the executor; it would still pass with the destroy() call removed, which is the wiring that was broken. Add a servlet test that configures the servlet, destroys it and checks the creator was shut down. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
asf-gitbox-commits
force-pushed
the
graphql-ws-scheduler-shutdown
branch
from
September 4, 2026 15:33
f5deaab to
07ffe7f
Compare
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.
Summary
SubscriptionWebSocketFactoryis only ever handed to Jetty as the socket creator and is never started or stopped as a lifecycle, so thedoStop()override that was meant to stop its deadline scheduler never ran and the scheduler thread outlived each servlet instance.GraphQLServlet.destroy(), via an explicitshutdown()on the creator, instead of relying on a lifecycle hook that is never invoked.Test plan
SubscriptionWebSocketFactoryTest.shutdown_stopsTheDeadlineScheduler;cxs-implunit tests green locally (57)🤖 Generated with Claude Code