-
Notifications
You must be signed in to change notification settings - Fork 1.2k
KTOR-9290 Jetty Jakarta idleTimeout does not cancel the request's job #5335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
KTOR-9290 Jetty Jakarta idleTimeout does not cancel the request's job #5335
Conversation
WalkthroughAdds idle-timeout-driven cancellation of the coroutine launched for request handling in the Jetty Jakarta handler and centralizes guarded error-write logic; adds a test that verifies idle timeout cancels the coroutine and unwraps the resulting CancellationException root cause. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| request, | ||
| response, | ||
| callback, | ||
| HttpStatus.GONE_410, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should say I'm not a fan of this 410 Gone here - I can't see how a cancellation exception should imply
the target resource is no longer available at the origin server and that this condition is likely to be permanent
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/410
408 probably fits better.. but in this context we can't be sure how this bubbled up so it could be a 500 like the others.
fb01cbd to
7ff857c
Compare
7ff857c to
85a3e27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@ktor-server/ktor-server-jetty-jakarta/jvm/test/io/ktor/tests/server/jetty/jakarta/JettyIdleTimeoutTest.kt`:
- Around line 39-41: The comment in JettyIdleTimeoutTest.kt contains a typo:
change "unhanded" to "unhandled" in the KDoc above the test (the block
mentioning afterTest override); locate the comment near the class/test that
overrides afterTest and correct the word to "unhandled" so the sentence reads
"how an unhandled CancellationException is handled" to fix the typo.
🧹 Nitpick comments (1)
ktor-server/ktor-server-jetty-jakarta/jvm/test/io/ktor/tests/server/jetty/jakarta/JettyIdleTimeoutTest.kt (1)
17-18: Redundant import:CancellationExceptionis already covered by the star import.Line 18 explicitly imports
kotlinx.coroutines.CancellationException, but Line 17's star importkotlinx.coroutines.*already includes it.🧹 Suggested fix
import kotlinx.coroutines.* -import kotlinx.coroutines.CancellationException
...tor-server-jetty-jakarta/jvm/test/io/ktor/tests/server/jetty/jakarta/JettyIdleTimeoutTest.kt
Show resolved
Hide resolved
…erver/jetty/jakarta/JettyIdleTimeoutTest.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Subsystem
Jetty Jakarta Ktor Server
Motivation
https://youtrack.jetbrains.com/issue/KTOR-9290/Jetty-Jakarta-idleTimeout-does-not-cancel-the-requests-job
When Jetty's idle timeout expires during request processing, the timeout is effectively ignored if the server-side coroutine is performing slow work. Clients would still receive the intended status code beyond the idle timeout time.
The current tests are focussed on clients being slow to read and write.
The fix adds an idle timeout listener to the Jetty request that cancels the handler's coroutine job when the timeout fires therefore making it effective on server resources.