KTOR-8528 Fix race condition when Netty removes headers for some resp… #4901
Conversation
WalkthroughAn internal suspend function was added to the Netty server response implementation to suspend coroutine processing for informational (1xx) or no-content (204) HTTP statuses after sending the response. A new test verifies that "Content-Length" and "Transfer-Encoding" headers are removed for 204 and 1xx responses. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettySpecificTest.kt (1)
90-171: Comprehensive test coverage with minor improvements needed.The test effectively validates the race condition fix by verifying header removal for informational and no-content responses across different response mechanisms. The use of
ResponseSenthook to assert header absence is well-designed.Consider these improvements:
Route path consistency: Some routes use leading slashes (
"/no-content","/info") while others don't ("no-content-channel-writer"). Standardize for consistency.Error handling: Add error handling for server startup failures in the launch block.
Test scope cleanup: Consider using
TestScopeor ensure proper cleanup of thetestScopecoroutine context.- get("no-content-channel-writer") { + get("/no-content-channel-writer") { call.respondBytesWriter(status = HttpStatusCode.NoContent) {} } - get("no-content-read-channel") { + get("/no-content-read-channel") { call.respond(object : OutgoingContent.ReadChannelContent() { override val status: HttpStatusCode = HttpStatusCode.NoContent override fun readFrom(): ByteReadChannel = ByteReadChannel.Empty }) } - get("info-channel-writer") { + get("/info-channel-writer") { call.respondBytesWriter(status = earlyHints) {} } - get("info-read-channel") { + get("/info-read-channel") { call.respond(object : OutgoingContent.ReadChannelContent() { override val status: HttpStatusCode = earlyHints override fun readFrom(): ByteReadChannel = ByteReadChannel.Empty }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt(4 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/cio/NettyHttpResponsePipeline.kt(1 hunks)ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettySpecificTest.kt(2 hunks)
🔇 Additional comments (5)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/cio/NettyHttpResponsePipeline.kt (1)
74-76: LGTM: Proper synchronization for race condition fix.The addition of
sendCompleted.complete(Unit)for informational and no-content responses in the finally block correctly addresses the race condition described in KTOR-8528. This ensures that response methods awaitingsendCompletedare properly resumed after the response processing is complete.ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt (4)
37-37: Well-designed lazy initialization for synchronization.Using
lazyfor thesendCompleteddeferred is efficient - it only creates the deferred when needed for informational or no-content responses.
73-76: Correct status code detection logic.The
isInfoOrNoContentStatus()function correctly identifies:
- HTTP 204 No Content responses
- Informational 1xx status codes (100-199 range)
This matches the HTTP specification requirements for responses that should not have content-related headers.
143-143: Improved exception specificity.Using
CancellationExceptioninstead of a generic exception provides better error semantics for response cancellation scenarios.
68-70:✅ Verification successful
Verify sendCompleted is always completed to prevent hanging.
The synchronization logic correctly suspends response methods for special status codes until the pipeline completes processing. However, ensure that
sendCompletedis always completed in all code paths to prevent indefinite suspension.Also applies to: 83-85
🏁 Script executed:
#!/bin/bash # Verify that sendCompleted.complete() is called in all relevant code paths # Expected: Find completion calls in NettyHttpResponsePipeline and potential error handling paths echo "=== Searching for sendCompleted.complete() calls ===" rg -A 3 -B 3 "sendCompleted\.complete" echo -e "\n=== Searching for sendCompleted usage ===" rg -A 5 -B 2 "sendCompleted" echo -e "\n=== Checking error handling in NettyHttpResponsePipeline ===" rg -A 10 -B 5 "respondWithFailure.*NettyApplicationCall"Length of output: 6966
sendCompleted completion verified
A search confirms thatsendCompleted.complete(Unit)is invoked in thefinallyblock of
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/cio/NettyHttpResponsePipeline.kt
for all INFO and NO_CONTENT status paths, guaranteeingsendCompleted.await()will always resume.
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt
Show resolved
Hide resolved
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a race condition in Netty where Content-Length and Transfer-Encoding headers could be removed too early for 204 (No Content) and 1xx (informational) responses by awaiting the response write job, and it adds a test to verify header removal.
- Await
responseWriteJobbefore headers are stripped inNettyApplicationResponse - Add
contentLengthAndTransferEncodingAreSafelyRemovedtest inNettySpecificTest
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettySpecificTest.kt | Added async test verifying headers are removed for 204 and 1xx responses |
| ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt | Added waiting logic for responseWriteJob on info/NoContent statuses and streamlined CancellationException import |
Comments suppressed due to low confidence (1)
ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettySpecificTest.kt:99
- [nitpick] The plugin name 'CallLogging' duplicates an existing Ktor plugin, which may cause confusion or conflicts. Consider renaming it to something more descriptive like 'ContentHeaderRemovalVerifier'.
createApplicationPlugin("CallLogging") {
ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettySpecificTest.kt
Show resolved
Hide resolved
| if (isInfoOrNoContentStatus()) { | ||
| if (call is NettyApplicationCall) { | ||
| (call as NettyApplicationCall).responseWriteJob.join() | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to await responseWriteJob for informational and NoContent statuses is duplicated in both sendResponse and responseChannel. Consider extracting this into a shared private method to reduce duplication and improve maintainability.
| if (isInfoOrNoContentStatus()) { | |
| if (call is NettyApplicationCall) { | |
| (call as NettyApplicationCall).responseWriteJob.join() | |
| } | |
| } | |
| awaitResponseWriteJobIfNeeded() |
| if (call is NettyApplicationCall) { | ||
| (call as NettyApplicationCall).responseWriteJob.join() | ||
| } |
There was a problem hiding this comment.
[nitpick] After checking 'call is NettyApplicationCall', you can simplify the cast by using a safe cast ('call as? NettyApplicationCall') and a safe call, eliminating the need for a separate instance check.
| if (call is NettyApplicationCall) { | |
| (call as NettyApplicationCall).responseWriteJob.join() | |
| } | |
| (call as? NettyApplicationCall)?.responseWriteJob?.join() |
e5l
left a comment
e5l
left a comment
There was a problem hiding this comment.
Could you explain in the PR body why the response headers were removed?
To avoid the race condition, we will wait for Netty to finish discarding certain headers like
Content-Lengthfor informational and 204 responses.https://youtrack.jetbrains.com/issue/KTOR-8528