Server. Detect if a request was cancelled by the client. #5181
Conversation
WalkthroughAdds per-request lifecycle support: new ConnectionClosedException, HttpRequestLifecycle plugin and attribute key, engine wiring to invoke request-close handlers (CIO and Netty), ServerRequestScope.onClose, API manifest updates, and tests validating cancellation on client disconnect, success, and streaming. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.kt (1)
62-190: Don’t cancel normalConnection: closerequests.
With the newrequestvar, we now invokerequest?.onClose?.invoke()unconditionally infinally. When a client sendsConnection: close(or HTTP/1.0 without keep-alive),isLastHttpRequest(...)triggers abreakeven though the request is healthy and the handler is still running. As soon as we exit the loop, thisfinallyblock fires, the close handler cancels the call withConnectionClosedException, and we abort a response that should have completed successfully. Please gate the callback so it only fires when the pipeline actually terminates because the peer dropped the input (EOF, IO failure, cancellation), not when we intentionally close after servicing the request. A simple fix is to track ashouldNotifyCloseflag that you set in the exceptional/EOF paths and check before invokingonClose.Apply this diff (sketching the guard):
@@ - var request: Request? = null + var request: Request? = null + var shouldNotifyClose = false @@ - request = try { - parseRequest(connection.input) ?: break + request = try { + parseRequest(connection.input) ?: run { + shouldNotifyClose = true + break + } } catch (cause: TooLongLineException) { respondBadRequest(actorChannel) break // end pipeline loop } catch (io: IOException) { + shouldNotifyClose = true throw io } catch (cancelled: CancellationException) { + shouldNotifyClose = true throw cancelled } catch (parseFailed: Throwable) { @@ - } finally { - request?.onClose?.invoke() + } finally { + if (shouldNotifyClose) { + request?.onClose?.invoke() + } actorChannel.close() }
♻️ Duplicate comments (1)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt (1)
117-117: Consider adding debug-level logging for observability.Same as line 57—the early return is silent. Consider adding a comment or debug log for observability.
🧹 Nitpick comments (2)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt (1)
57-57: Consider adding debug-level logging for observability.The early return when
!canRespondis silent, which could make debugging harder if the channel is in an unexpected state. Consider adding a debug log to track when response sending is skipped.Example:
- if (!canRespond) return + if (!canRespond) { + // Client disconnected or response already sent + return + }ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (1)
19-19: Remove redundant explicit import.The explicit import of
CancellationExceptionis unnecessary since line 18 already importskotlinx.coroutines.*.Apply this diff to remove the redundant import:
import kotlinx.coroutines.* -import kotlinx.coroutines.CancellationException import kotlin.test.Test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.kt(2 hunks)ktor-io/common/src/io/ktor/utils/io/Exceptions.kt(1 hunks)ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.kt(3 hunks)ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.kt(2 hunks)ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt(3 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt(3 hunks)ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt(1 hunks)ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{kt,kts}: Follow Kotlin official style guide for all Kotlin source and build scripts
Use star imports for io.ktor.* packages
Max line length is 120 characters
Indent with 4 spaces in Kotlin code
Include a copyright header in new Kotlin files
Files:
ktor-io/common/src/io/ktor/utils/io/Exceptions.ktktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.ktktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.ktktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.ktktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Document all public Kotlin APIs, including parameters, return types, and exceptions
Annotate internal APIs with @internalapi
Follow Kotlin error-handling conventions and use specific Ktor exceptions
Files:
ktor-io/common/src/io/ktor/utils/io/Exceptions.ktktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.ktktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.ktktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.ktktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
🧠 Learnings (15)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: In the Ktor DI plugin, when a DependencyRegistry implements CoroutineScope, it should be properly cancelled during application shutdown to prevent memory leaks and resource issues. This is handled by calling registry.cancel() in the ApplicationStopped event.
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: When implementing CoroutineScope in Ktor components like DependencyRegistry, always ensure the scope is properly cancelled during application shutdown to prevent memory leaks and resource issues. In the DI plugin, this is handled by calling registry.cancel() in the ApplicationStopped event handler after all dependency cleanup is complete.
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: Jetty's AbstractConnection class implements Closeable interface, so any class extending AbstractConnection (like JettyWebsocketConnection) can be used with Kotlin's use { } extension function for resource management.
Applied to files:
ktor-io/common/src/io/ktor/utils/io/Exceptions.ktktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Applies to **/*.kt : Follow Kotlin error-handling conventions and use specific Ktor exceptions
Applied to files:
ktor-io/common/src/io/ktor/utils/io/Exceptions.ktktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.
Applied to files:
ktor-io/common/src/io/ktor/utils/io/Exceptions.ktktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: JettyWebsocketConnection in ktor-server-jetty-jakarta implements Closeable interface (has close() method), so connection.use { } is valid Kotlin syntax and will compile correctly.
Applied to files:
ktor-io/common/src/io/ktor/utils/io/Exceptions.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-10-15T16:47:28.272Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 5125
File: ktor-shared/ktor-openapi-schema/common/src/io/ktor/openapi/Operation.kt:442-444
Timestamp: 2025-10-15T16:47:28.272Z
Learning: In the ktor-openapi-schema module, the maintainer prefers to keep certain properties mutable (using `var` instead of `val`) in data classes like Response, even when they are part of the public API, to allow flexibility during construction and modification.
Applied to files:
ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Applies to **/*.kt : Annotate internal APIs with InternalAPI
Applied to files:
ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.kt
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Applies to **/*.{kt,kts} : Use star imports for io.ktor.* packages
Applied to files:
ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-06-16T17:03:45.729Z
Learnt from: tre3p
Repo: ktorio/ktor PR: 4936
File: ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt:28-35
Timestamp: 2025-06-16T17:03:45.729Z
Learning: In the Ktor codebase, encoder implementations consistently use GlobalScope.writer and GlobalScope.reader patterns for compression/decompression operations. This is the established architectural pattern used throughout the codebase for byte channel transformations.
Applied to files:
ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: The headersOf() function from io.ktor.http package is available through wildcard imports like `import io.ktor.http.*`, so no explicit import statement is needed when using wildcard imports from that package.
Applied to files:
ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-09-05T12:46:14.074Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:90-100
Timestamp: 2025-09-05T12:46:14.074Z
Learning: The ByteReadChannel.readAvailable(ByteBuffer) method in Ktor IO automatically calls awaitContent() internally when the read buffer is exhausted. When it returns 0, it has already suspended and waited for data to become available, so adding explicit awaitContent() calls is redundant and incorrect.
Applied to files:
ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt
📚 Learning: 2025-09-30T07:52:14.769Z
Learnt from: yschimke
Repo: ktorio/ktor PR: 4013
File: ktor-client/ktor-client-android/jvm/src/io/ktor/client/engine/android/Android14URLConnectionFactory.kt:24-26
Timestamp: 2025-09-30T07:52:14.769Z
Learning: In the Ktor Android HTTP client engine (ktor-client-android), prefer using `URI.create(urlString).toURL()` over the `URL(urlString)` constructor when opening connections with Android's HttpEngine, as it avoids deprecated APIs and the different exception behavior (IllegalArgumentException vs MalformedURLException) is acceptable.
Applied to files:
ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Applied to files:
ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.ktktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt
📚 Learning: 2025-05-14T18:17:03.059Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: When implementing CoroutineScope in Ktor components like DependencyRegistry, always ensure the scope is properly cancelled during application shutdown to prevent memory leaks and resource issues. In the DI plugin, this is handled by calling registry.cancel() in the ApplicationStopped event handler after all dependency cleanup is complete.
Applied to files:
ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.ktktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
📚 Learning: 2025-10-22T07:21:51.263Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 5139
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyInjection.kt:11-11
Timestamp: 2025-10-22T07:21:51.263Z
Learning: In Ktor, `io.ktor.utils.io.CancellationException` is a typealias for `kotlinx.coroutines.CancellationException`, which is a supertype of `JobCancellationException`. Therefore, checking `e !is io.ktor.utils.io.CancellationException` is sufficient to exclude all coroutine cancellation exceptions.
Applied to files:
ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.kt
🧬 Code graph analysis (1)
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt (1)
ktor-utils/common/src/io/ktor/util/Attributes.kt (1)
AttributeKey(19-21)
🔇 Additional comments (7)
ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt (1)
390-392: Nice to see Netty in the lifecycle suite.
Thanks for wiring Netty into the sharedHttpRequestLifecycleTest; that coverage will keep the new close-handling behaviour honest across engines.ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.kt (1)
174-201: Good hook-up of the close handler.
LinkingRequest.onCloseback to the per-call attribute keeps the CIO engine aligned with the new lifecycle plugin without leaking engine details into the plugin. Nicely done.ktor-io/common/src/io/ktor/utils/io/Exceptions.kt (1)
32-38: Custom exception fits the use case.
ConnectionClosedExceptiongives us a clear, typed signal for disconnects instead of overloading generic IO errors. Looks good.ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.kt (1)
59-62:onCloseextension point makes sense.
Exposing an@InternalAPIclose hook onRequestkeeps the CIO pipeline extensible without affecting public APIs. Looks solid.ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt (1)
89-94: Great to see CIO covered too.
Registering the CIO engine with the shared lifecycle test suite keeps parity with Netty and ensures regressions surface quickly.ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt (1)
39-87: Netty close wiring looks good.
Stashing the callback per request and invoking it fromchannelInactivekeeps the behaviour symmetric with CIO. Thanks for cleaning up the lifecycle path here.ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (1)
26-28: LGTM!The abstract test class declaration properly extends
EngineTestBasewith appropriate generic constraints for testing different engine implementations.
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
Show resolved
Hide resolved
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt
Outdated
Show resolved
Hide resolved
...er/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt
Outdated
Show resolved
Hide resolved
...er/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt
Outdated
Show resolved
Hide resolved
...er/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt
Outdated
Show resolved
Hide resolved
bjhham
left a comment
bjhham
left a comment
There was a problem hiding this comment.
Looks good, just have a couple comments.
| @InternalAPI | ||
| public var onClose: (() -> Unit)? = null |
There was a problem hiding this comment.
Maybe we can drop the field and call the attribute directly from an extension function:
@InternalApi
fun Request.onClose() {
call.attributes.getOrNull(HttpRequestCloseHandlerKey)?.invoke()
}
There was a problem hiding this comment.
Unfortunately, we can't access call from Request.
On the other hand, we just need onClose in CIOApplicationEngine, but it's created in cio.backend.ServerPipeline. Another way to pass it could be through ServerRequestScope(in this case, onClose can be internal and won't modify public API).
There was a problem hiding this comment.
I moved it in the second commit.
There was a problem hiding this comment.
How about using something like ServerRequestScope.coroutingContext[Job].invokeOnCompletion {} or possibly launchOnCancellation?
| import kotlin.time.Duration.Companion.milliseconds | ||
| import kotlin.time.Duration.Companion.seconds | ||
|
|
||
| abstract class HttpRequestLifecycleTest<TEngine : ApplicationEngine, TConfiguration : ApplicationEngine.Configuration>( |
There was a problem hiding this comment.
It seems this test is a bit flaky on the CI. It might just need an extended timeout.
There was a problem hiding this comment.
If you mean the test that failed Java 8, 11 on Linux, it's actually io.ktor.tests.server.netty.NettyConfigurationTest.configuredChannelEventuallyClosed()[jvm], not my test.
Anyway, it makes sense to increase the timeout to 10 seconds (in case of high load on the CI).
There was a problem hiding this comment.
Nah, I mean this test in particular failed but the automatic retry got it through. Unfortunately, we don't have any kind of reporting on when tests are retried so the only way to notice is by watching them as they're running 😓
There was a problem hiding this comment.
Ohh, I didn't know that. I'll look in the logs; maybe there is a real problem.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.kt (1)
62-191: Cancel all in-flight requests, not just the last one
handlerScopeis a single mutable slot that gets overwritten on every new request. When CIO pipelines multiple requests on the same connection (seeEndpointConfig.pipelineMaxSize/ Netty’srunningLimit), a client disconnect only triggersonClosefor the most recently assigned scope. Any earlier handlers that are still executing keep running and never observe the cancellation signal, soHttpRequestLifecyclesilently fails for pipelined workloads.Please track every active
ServerRequestScopeand invoke each storedonClosewhen the connection dies. One way is to keep a mutex-protected set of scopes, add/remove them inside the per-request coroutine, and iterate over a snapshot in thefinallyblock:@@ -import kotlinx.coroutines.channels.* +import kotlinx.coroutines.channels.* +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock @@ - var handlerScope: ServerRequestScope? = null + val activeRequestsMutex = Mutex() + val activeRequests = mutableSetOf<ServerRequestScope>() @@ - launch(requestContext, start = CoroutineStart.UNDISPATCHED) { - handlerScope = ServerRequestScope( + launch(requestContext, start = CoroutineStart.UNDISPATCHED) { + val scope = ServerRequestScope( coroutineContext, requestBody, response, connection.remoteAddress, connection.localAddress, upgraded ) + activeRequestsMutex.withLock { activeRequests += scope } try { - handler(handlerScope, request) + handler(scope, request) @@ } finally { response.close() upgraded?.complete(false) + activeRequestsMutex.withLock { activeRequests -= scope } } } @@ - } finally { - handlerScope?.onClose?.invoke() + } finally { + val toCancel = activeRequestsMutex.withLock { activeRequests.toList() } + toCancel.forEach { it.onClose?.invoke() } actorChannel.close() }
🧹 Nitpick comments (5)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt (2)
52-69: Guard correctly prevents responses after disconnection.The
!canRespondcheck appropriately prevents sending a response when the client has disconnected or a response was already sent. The silent return (without throwing) aligns with gracefully handling client cancellations.Consider adding a debug log statement before the early return to aid troubleshooting scenarios where responses are unexpectedly not sent:
- if (!canRespond) return + if (!canRespond) { + // Response skipped: either already sent or channel inactive (client disconnected) + return + }
116-130: Guard correctly prevents responses after disconnection.The
!canRespondcheck consistently prevents sending a response when the channel is inactive or a response was already sent. This implementation aligns with the PR objective of detecting and gracefully handling client disconnections.Similar to
respondFromBytes, consider adding a brief inline comment before the early return to clarify the intentional silent skip for debugging purposes:- if (!canRespond) return + if (!canRespond) { + // Response skipped: either already sent or channel inactive (client disconnected) + return + }ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.kt (2)
9-9: Consider using star imports for io.ktor. packages.*Lines 9 and 14 use explicit imports for
io.ktor.http.cio.Requestandio.ktor.server.http.HttpRequestCloseHandlerKey. Per coding guidelines, io.ktor.* packages should use star imports.As per coding guidelines.
Apply this diff:
-import io.ktor.http.cio.Request +import io.ktor.http.cio.*-import io.ktor.server.http.HttpRequestCloseHandlerKey +import io.ktor.server.http.*Also applies to: 14-14
174-180: Consider wrapping handler invocation in error handling.The close handler invocation at line 178 is not protected by error handling. If the handler throws an exception, it could disrupt the request close flow or leave resources in an inconsistent state.
Apply this diff to add defensive error handling:
@OptIn(InternalAPI::class) private fun ServerRequestScope.setCloseHandler(call: CIOApplicationCall) { onClose = { - val requestCloseHandler = call.attributes.getOrNull(HttpRequestCloseHandlerKey) - requestCloseHandler?.invoke() + try { + val requestCloseHandler = call.attributes.getOrNull(HttpRequestCloseHandlerKey) + requestCloseHandler?.invoke() + } catch (cause: Throwable) { + // Log but don't propagate to allow cleanup to continue + call.application.environment.log.error("Error in request close handler", cause) + } } }ktor-io/api/ktor-io.api (1)
229-234: Exception design looks good; cause support is optional enhancement.Current implementation with
ConnectionClosedException(message: String = "Connection was closed")is correct and functional. The single usage site (HttpRequestLifecycle.kt:97) doesn't require cause chaining. Adding a cause constructor(String?, Throwable?)would follow IOException best practices, but it's not critical since the exception is primarily used as a marker signal rather than for wrapping underlying errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.kt(1 hunks)ktor-io/api/ktor-io.api(1 hunks)ktor-io/api/ktor-io.klib.api(1 hunks)ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.kt(3 hunks)ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.kt(4 hunks)ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerRequestScope.kt(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.api(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.klib.api(2 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt(3 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt(3 hunks)ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/RequestResponse.kt
🧰 Additional context used
📓 Path-based instructions (3)
**/api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Track all public API changes in the /api/ directories
Files:
ktor-io/api/ktor-io.klib.apiktor-server/ktor-server-core/api/ktor-server-core.klib.apiktor-io/api/ktor-io.apiktor-server/ktor-server-core/api/ktor-server-core.api
**/*.{kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{kt,kts}: Follow Kotlin official style guide for all Kotlin source and build scripts
Use star imports for io.ktor.* packages
Max line length is 120 characters
Indent with 4 spaces in Kotlin code
Include a copyright header in new Kotlin files
Files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerRequestScope.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.kt
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Document all public Kotlin APIs, including parameters, return types, and exceptions
Annotate internal APIs with @internalapi
Follow Kotlin error-handling conventions and use specific Ktor exceptions
Files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerRequestScope.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.kt
🧠 Learnings (14)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: In the Ktor DI plugin, when a DependencyRegistry implements CoroutineScope, it should be properly cancelled during application shutdown to prevent memory leaks and resource issues. This is handled by calling registry.cancel() in the ApplicationStopped event.
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: When implementing CoroutineScope in Ktor components like DependencyRegistry, always ensure the scope is properly cancelled during application shutdown to prevent memory leaks and resource issues. In the DI plugin, this is handled by calling registry.cancel() in the ApplicationStopped event handler after all dependency cleanup is complete.
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: Jetty's AbstractConnection class implements Closeable interface, so any class extending AbstractConnection (like JettyWebsocketConnection) can be used with Kotlin's use { } extension function for resource management.
Applied to files:
ktor-io/api/ktor-io.klib.apiktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-io/api/ktor-io.apiktor-server/ktor-server-core/api/ktor-server-core.apiktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-10-22T07:21:51.263Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 5139
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyInjection.kt:11-11
Timestamp: 2025-10-22T07:21:51.263Z
Learning: In Ktor, `io.ktor.utils.io.CancellationException` is a typealias for `kotlinx.coroutines.CancellationException`, which is a supertype of `JobCancellationException`. Therefore, checking `e !is io.ktor.utils.io.CancellationException` is sufficient to exclude all coroutine cancellation exceptions.
Applied to files:
ktor-io/api/ktor-io.klib.apiktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-io/api/ktor-io.api
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Applies to **/*.kt : Follow Kotlin error-handling conventions and use specific Ktor exceptions
Applied to files:
ktor-io/api/ktor-io.klib.apiktor-server/ktor-server-core/api/ktor-server-core.klib.apiktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-io/api/ktor-io.apiktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: JettyWebsocketConnection in ktor-server-jetty-jakarta implements Closeable interface (has close() method), so connection.use { } is valid Kotlin syntax and will compile correctly.
Applied to files:
ktor-io/api/ktor-io.klib.apiktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-io/api/ktor-io.apiktor-server/ktor-server-core/api/ktor-server-core.apiktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.kt
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.
Applied to files:
ktor-io/api/ktor-io.klib.apiktor-server/ktor-server-core/api/ktor-server-core.klib.apiktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-io/api/ktor-io.apiktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: The headersOf() function from io.ktor.http package is available through wildcard imports like `import io.ktor.http.*`, so no explicit import statement is needed when using wildcard imports from that package.
Applied to files:
ktor-server/ktor-server-core/api/ktor-server-core.klib.apiktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Applies to **/*.{kt,kts} : Use star imports for io.ktor.* packages
Applied to files:
ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-05-14T18:17:03.059Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: When implementing CoroutineScope in Ktor components like DependencyRegistry, always ensure the scope is properly cancelled during application shutdown to prevent memory leaks and resource issues. In the DI plugin, this is handled by calling registry.cancel() in the ApplicationStopped event handler after all dependency cleanup is complete.
Applied to files:
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerRequestScope.ktktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.kt
📚 Learning: 2025-05-14T18:17:03.059Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: In the Ktor DI plugin, when a DependencyRegistry implements CoroutineScope, it should be properly cancelled during application shutdown to prevent memory leaks and resource issues. This is handled by calling registry.cancel() in the ApplicationStopped event.
Applied to files:
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
📚 Learning: 2025-10-15T16:47:28.272Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 5125
File: ktor-shared/ktor-openapi-schema/common/src/io/ktor/openapi/Operation.kt:442-444
Timestamp: 2025-10-15T16:47:28.272Z
Learning: In the ktor-openapi-schema module, the maintainer prefers to keep certain properties mutable (using `var` instead of `val`) in data classes like Response, even when they are part of the public API, to allow flexibility during construction and modification.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt
📚 Learning: 2025-09-05T12:46:14.074Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:90-100
Timestamp: 2025-09-05T12:46:14.074Z
Learning: The ByteReadChannel.readAvailable(ByteBuffer) method in Ktor IO automatically calls awaitContent() internally when the read buffer is exhausted. When it returns 0, it has already suspended and waited for data to become available, so adding explicit awaitContent() calls is redundant and incorrect.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Applied to files:
ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.kt
🧬 Code graph analysis (2)
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt (1)
ktor-utils/common/src/io/ktor/util/Attributes.kt (1)
AttributeKey(19-21)
ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerPipeline.kt (1)
ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/HttpParser.kt (1)
parseRequest(38-68)
🔇 Additional comments (6)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationResponse.kt (1)
36-37: LGTM! Correctly usingisActive()to detect channel closure.The implementation properly uses
context.channel().isActiveto check if the channel is still open and connected. This correctly addresses the previous concern about usingisWritable(), which would have conflated backpressure with actual disconnection.The dual check (
!responseMessageSent && context.channel().isActive) ensures responses are only sent when appropriate: no prior response sent and the client is still connected.ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/CIOApplicationEngine.kt (1)
199-199: Proper placement of setCloseHandler before pipeline execution.The close handler is correctly registered before
pipeline.execute(call), ensuring the onClose callback is available during request processing.ktor-server/ktor-server-cio/common/src/io/ktor/server/cio/backend/ServerRequestScope.kt (1)
46-46: Clean addition of internal close callback mechanism.The
onCloseproperty provides a simple, internal hook for per-request cleanup. The nullable callback design is appropriate and the internal visibility correctly limits its scope to the CIO backend implementation.ktor-io/api/ktor-io.klib.api (1)
239-241: Well-designed exception for connection closure scenarios.The new
ConnectionClosedExceptionappropriately extendsIOExceptionand provides a default message, making it convenient to throw when detecting client disconnection. The public API addition is properly tracked in the API dump file.ktor-server/ktor-server-core/api/ktor-server-core.klib.api (2)
654-659: No changes required—KDoc and default already documented.The property already has comprehensive documentation addressing all concerns:
- Default value is explicitly documented:
Boolean = falsewith KDoc stating "Whenfalse(default)..."- Intent is clear: cancels the call coroutine if the peer resets the connection
- Close event scope is adequately described as connection resets (covering client disconnects and HTTP/2 stream resets)
1682-1686: No action needed—the key is already marked @internalapi.The review comment's primary suggestion to mark
HttpRequestCloseHandlerKeyas@InternalAPIis already implemented in the source code (line 35 ofHttpRequestLifecycle.kt). The annotation prevents public API misuse as intended.Regarding the suggestion to change the signature to
(Throwable?) -> Unit: the current closure-based design is sound. The handler is instantiated with the necessary cause already captured (CancellationExceptionwrappingConnectionClosedException), so engines don't need to pass parameters—they simply invoke the handler when the connection closes. Both Netty and CIO engines follow this pattern, and there's no evidence of friction with the current approach.
marychatte
left a comment
marychatte
left a comment
There was a problem hiding this comment.
lgtm, I have a question, should it also work for HTTP/2 in Netty?
| * cancelCallOnClose = true | ||
| * } | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
It's not clear from the documentation where the cancellation exception will be thrown. Could you please add an example of handling the exception as well?
There was a problem hiding this comment.
It is also unclear what the use case for the new property is - when should someone use it?
| * | ||
| * ```kotlin | ||
| * install(HttpRequestLifecycle) { | ||
| * cancelCallOnClose = true |
There was a problem hiding this comment.
I wonder if this should be the default, and we don't need an option to change it. @bjhham, what do you think?
There was a problem hiding this comment.
This was the initial idea, but iirc @zibet27 mentioned it broke quite a few tests so it appears it could be quite impactful. There's not many scenarios for keeping the call coroutine alive after the connection is lost, but they do exist. Maybe the best approach would be to log a ticket to enable it by default in 4.0.
There was a problem hiding this comment.
@e5l I think the cancellation is needed only for computation-heavy requests because it can cancel a DB insert, which is usually not what you want, even if the client is not waiting for a response.
There was a problem hiding this comment.
It would be pretty uncommon for a client to hang up before receiving a response like this though. Maybe if the DB is under load and the client has a short timeout though... I agree though, it's probably safer to assume that there could be sensitive work being done in the call coroutine. The main use-case for cancellation would be long-polling connections I guess.
| * Internal attribute key for storing the connection close handler callback. | ||
| */ | ||
| @InternalAPI | ||
| public val HttpRequestCloseHandlerKey: AttributeKey<() -> Unit> = AttributeKey<() -> Unit>("HttpRequestCloseHandler") |
There was a problem hiding this comment.
I would make a class to store the config instead of using a callback type. It will have name with clear semantic
There was a problem hiding this comment.
@e5l
We can have the HttpRequestLifecycleHandler class with the onClose method.
I'm not sure about this. Moreover, it's internal.
It should, though not tested, I'll add some tests. |
| upgraded | ||
| ) | ||
|
|
||
| internal var onClose: (() -> Unit)? = null |
There was a problem hiding this comment.
Hey, could you check if using invokeOnCompletion for the job is viable here instead of introducing the field?
There was a problem hiding this comment.
Hey! Sorry, I forgot to mention it here. invokeOnCompletion doesn't help in this case.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt (3)
286-299: Per-call Apache client in withHttp2 is OK for tests but could be reused if it ever gets hot
withHttp2now constructs a fresh ApacheHttpClientfor each call and closes it viause. That’s perfectly fine for occasional test calls; if this ever ends up in tight loops or high‑volume suites, consider reusing a single HTTP/2 client instead to avoid repeated engine setup/teardown overhead.
301-325: TLS keystore & CIO client wiring looks good; sslContext in createTrustManager is redundantThe lazy
keyStorepluscreateTrustManager()/createCIOClient()setup cleanly aligns client trust with the test keystore and keeps the configuration centralized; this is a nice improvement. TheSSLContextallocated insidecreateTrustManager()is currently only initialized and never used, so you can safely drop it and construct theX509TrustManagerdirectly fromTrustManagerFactoryif you want to trim a bit of noise.- fun createTrustManager(): X509TrustManager { - val sslContext = SSLContext.getInstance("TLS") - val tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()) - tmf.init(keyStore) - sslContext.init(null, tmf.trustManagers, null) - return tmf.trustManagers.first { it is X509TrustManager } as X509TrustManager - } + fun createTrustManager(): X509TrustManager { + val tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()) + tmf.init(keyStore) + return tmf.trustManagers.first { it is X509TrustManager } as X509TrustManager + }
339-343: Default CIO client from setupAll can be leaked when tests overwriteclient
setupAll()unconditionally doesclient = createCIOClient(), but some tests (for example,HttpRequestLifecycleTest.resetRequestOnStartinHttpRequestLifecycleTest.kt) overwriteclientwithcreateApacheClient()and then close only the new client viaclient.use { ... }. That pattern loses the original CIO client reference created here, so it never gets closed;cleanup()will only close whateverclientwas last assigned.This is low impact since it’s test-only, but for better resource hygiene (and in line with the general “close/cancel scopes on shutdown” pattern in Ktor tests, based on learnings), consider:
- Either closing the previous
clientbefore reassigning it in such tests, or- Introducing a small helper in this companion that closes the existing client before swapping it out, and using that instead of raw
client = ....For example, in this file:
- lateinit var client: HttpClient + lateinit var client: HttpClient + + private fun replaceClient(newClient: HttpClient) { + if (::client.isInitialized) { + client.close() + } + client = newClient + } @@ - fun setupAll() { - client = createCIOClient() - } + fun setupAll() { + replaceClient(createCIOClient()) + }Tests that need a custom client could then call
replaceClient(createApacheClient())instead of assigningclientdirectly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt(4 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.kt(3 hunks)ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt(1 hunks)ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt(5 hunks)ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/ConfigTestSuite.kt(1 hunks)ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: In the Ktor DI plugin, when a DependencyRegistry implements CoroutineScope, it should be properly cancelled during application shutdown to prevent memory leaks and resource issues. This is handled by calling registry.cancel() in the ApplicationStopped event.
📚 Learning: 2025-05-14T18:17:03.059Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: When implementing CoroutineScope in Ktor components like DependencyRegistry, always ensure the scope is properly cancelled during application shutdown to prevent memory leaks and resource issues. In the DI plugin, this is handled by calling registry.cancel() in the ApplicationStopped event handler after all dependency cleanup is complete.
Applied to files:
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
📚 Learning: 2025-05-14T18:17:03.059Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: In the Ktor DI plugin, when a DependencyRegistry implements CoroutineScope, it should be properly cancelled during application shutdown to prevent memory leaks and resource issues. This is handled by calling registry.cancel() in the ApplicationStopped event.
Applied to files:
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/ConfigTestSuite.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-11-14T14:11:30.292Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 5195
File: ktor-client/ktor-client-java/jvm/test/io/ktor/client/engine/java/JavaHttp2Test.kt:13-15
Timestamp: 2025-11-14T14:11:30.292Z
Learning: In ktor-client-java tests, Java's HttpClient does support h2c (HTTP/2 cleartext) when configured with `protocolVersion = HttpClient.Version.HTTP_2` on Java 11+. The JavaHttp2Test successfully passes with the default useH2c=true setting, connecting to http://localhost:8084.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: JettyWebsocketConnection in ktor-server-jetty-jakarta implements Closeable interface (has close() method), so connection.use { } is valid Kotlin syntax and will compile correctly.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: Jetty's AbstractConnection class implements Closeable interface, so any class extending AbstractConnection (like JettyWebsocketConnection) can be used with Kotlin's use { } extension function for resource management.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: The headersOf() function from io.ktor.http package is available through wildcard imports like `import io.ktor.http.*`, so no explicit import statement is needed when using wildcard imports from that package.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-05-16T13:11:28.416Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4860
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.api:24-26
Timestamp: 2025-05-16T13:11:28.416Z
Learning: Binary incompatible changes (such as constructor signature changes) are acceptable in the ktor-server-di module when the version is not yet released, as confirmed by the development team.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-09-30T07:52:14.769Z
Learnt from: yschimke
Repo: ktorio/ktor PR: 4013
File: ktor-client/ktor-client-android/jvm/src/io/ktor/client/engine/android/Android14URLConnectionFactory.kt:24-26
Timestamp: 2025-09-30T07:52:14.769Z
Learning: In the Ktor Android HTTP client engine (ktor-client-android), prefer using `URI.create(urlString).toURL()` over the `URL(urlString)` constructor when opening connections with Android's HttpEngine, as it avoids deprecated APIs and the different exception behavior (IllegalArgumentException vs MalformedURLException) is acceptable.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-06-16T17:03:45.729Z
Learnt from: tre3p
Repo: ktorio/ktor PR: 4936
File: ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt:28-35
Timestamp: 2025-06-16T17:03:45.729Z
Learning: In the Ktor codebase, encoder implementations consistently use GlobalScope.writer and GlobalScope.reader patterns for compression/decompression operations. This is the established architectural pattern used throughout the codebase for byte channel transformations.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module depends on the kotlin("plugin.atomicfu") plugin, so atomicfu should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Gobley Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module requires the kotlin("plugin.atomicfu") plugin as a dependency, so the atomicfu plugin should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
🧬 Code graph analysis (3)
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt (1)
ktor-utils/common/src/io/ktor/util/Attributes.kt (1)
AttributeKey(19-21)
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (1)
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt (5)
createAndStartServer(165-197)createApacheClient(326-337)withHttp1(272-284)withHttp2(286-299)withUrl(246-260)
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt (1)
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (1)
engine(28-153)
🔇 Additional comments (11)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.kt (1)
163-163: LGTM! Idiomatic Kotlin improvement.Replacing named exception parameters with
_is the appropriate Kotlin convention when the caught exception is not used in the catch block. This makes the code more readable by explicitly showing that the exception value is intentionally ignored while maintaining the same error-handling behavior.Also applies to: 177-177, 190-190
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt (2)
17-36: LGTM!The configuration class design is sound. The internal constructor ensures controlled instantiation, and the comprehensive documentation with clear use-case guidance helps users make informed decisions about enabling cancellation.
91-105: Good fix for duplicate registration issue.The guard on lines 92-96 correctly prevents the
DuplicateAttributeKeyExceptionthat was flagged in previous reviews. The early return whencall.attributes.contains(HttpRequestCloseHandlerKey)ensures the handler is only registered once, even if the plugin is installed on multiple route levels.ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt (1)
90-94: LGTM!The integration with the lifecycle mechanism is correct. The handler is properly notified of the connection closure via
onConnectionClosebefore propagating the inactive event through the pipeline.ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt (1)
390-396: LGTM!The test class correctly extends the abstract lifecycle test suite for Netty with appropriate SSL and HTTP/2 configuration, following the established pattern in this file.
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (3)
32-39: LGTM!The helper function provides a clean abstraction for setting up the cancellable route configuration used across the tests.
113-119: Good resource management.The use of
client.use {}ensures the HttpClient is properly closed even if assertions fail within the block, addressing the resource leak concerns from previous reviews.
140-147: Good resource management.The use of
client.use {}ensures proper cleanup of the HttpClient, addressing the resource leak concerns from previous reviews.ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt (3)
246-260: Refactoring withUrl to delegate via withHttp1/withHttp2 looks correctDelegating through
withHttp1/withHttp2keeps the existing HTTP/1 + HTTPS + HTTP/2 sequencing and centralizes URL/client logic; no behavioral issues spotted here.
272-284: withHttp1 helper correctly reuses the shared clientThe new
withHttp1helper wiresbuilderandblockthrough toprepareRequest().executewhile propagating theportparameter; this matches the previouswithUrlbehavior and looks sound.
326-337: Apache client + trustAllCertificates helper is appropriate for test-only usageThe
createApacheClient()helper combined with the localtrustAllCertificatesX509TrustManagercleanly isolates “trust everything” behavior to this test base, which is appropriate for self-signed test keystores and avoids leaking insecure defaults into production code. Just be aware that this bypasses any certificate verification knobs (e.g.,enableCertVerify) on the client side—if you later need tests that exercise strict verification, you’ll likely want a second, verification-enabled client factory.Also applies to: 363-370
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
Outdated
Show resolved
Hide resolved
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
Show resolved
Hide resolved
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/ConfigTestSuite.kt
Show resolved
Hide resolved
...er/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt
Show resolved
Hide resolved
zibet27
force-pushed
the
zibet27/request-cancellation
branch
from
0c95925 to
be42a6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/ConfigTestSuite.kt (1)
60-62: Mutable shared state causes test flakiness on repeated runs.The companion object
countpersists across test invocations. The assertion on line 28 will fail iftestStartOnceWhenExceptionruns multiple times (e.g., during repeated test runs).Add a
@BeforeTestmethod to reset the counter:+ @BeforeTest + fun resetCount() { + count = 0 + } + private companion object { var count = 0 }Alternatively, convert
countto an instance variable if the lambda capture on line 20 doesn't affect the autoreload behavior being tested (per the comment on lines 17-18).ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt (1)
29-29: Thread-safety issue:currentCallrequires synchronization.The
currentCallfield is accessed from multiple methods (handleRequestat line 54,onConnectionCloseat lines 44-45) that execute oncallEventGroupexecutor threads without synchronization. SinceNettyApplicationCallHandleris added to the pipeline withaddLast(callEventGroup, ...)(as seen in NettyHttp1Handler.kt:51-52), andcallEventGroupcan contain multiple threads, this creates a race condition.Apply this diff to make the field thread-safe:
- private var currentCall: PipelineCall? = null + @Volatile + private var currentCall: PipelineCall? = nullNote: If you need atomic operations that coordinate both
currentCallandcurrentJob, consider using explicit locking instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt(4 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.kt(3 hunks)ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt(1 hunks)ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt(5 hunks)ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/ConfigTestSuite.kt(1 hunks)ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
- ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.kt
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: In the Ktor DI plugin, when a DependencyRegistry implements CoroutineScope, it should be properly cancelled during application shutdown to prevent memory leaks and resource issues. This is handled by calling registry.cancel() in the ApplicationStopped event.
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/ConfigTestSuite.ktktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-11-14T14:11:30.292Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 5195
File: ktor-client/ktor-client-java/jvm/test/io/ktor/client/engine/java/JavaHttp2Test.kt:13-15
Timestamp: 2025-11-14T14:11:30.292Z
Learning: In ktor-client-java tests, Java's HttpClient does support h2c (HTTP/2 cleartext) when configured with `protocolVersion = HttpClient.Version.HTTP_2` on Java 11+. The JavaHttp2Test successfully passes with the default useH2c=true setting, connecting to http://localhost:8084.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: JettyWebsocketConnection in ktor-server-jetty-jakarta implements Closeable interface (has close() method), so connection.use { } is valid Kotlin syntax and will compile correctly.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: Jetty's AbstractConnection class implements Closeable interface, so any class extending AbstractConnection (like JettyWebsocketConnection) can be used with Kotlin's use { } extension function for resource management.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-06-09T07:08:35.085Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4916
File: ktor-server/ktor-server-core/api/ktor-server-core.api:151-151
Timestamp: 2025-06-09T07:08:35.085Z
Learning: Breaking changes are acceptable in Ktor codebase when the code hasn't been released yet, as confirmed by bjhham from the development team.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: The headersOf() function from io.ktor.http package is available through wildcard imports like `import io.ktor.http.*`, so no explicit import statement is needed when using wildcard imports from that package.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-09-30T07:52:14.769Z
Learnt from: yschimke
Repo: ktorio/ktor PR: 4013
File: ktor-client/ktor-client-android/jvm/src/io/ktor/client/engine/android/Android14URLConnectionFactory.kt:24-26
Timestamp: 2025-09-30T07:52:14.769Z
Learning: In the Ktor Android HTTP client engine (ktor-client-android), prefer using `URI.create(urlString).toURL()` over the `URL(urlString)` constructor when opening connections with Android's HttpEngine, as it avoids deprecated APIs and the different exception behavior (IllegalArgumentException vs MalformedURLException) is acceptable.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-06-16T17:03:45.729Z
Learnt from: tre3p
Repo: ktorio/ktor PR: 4936
File: ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt:28-35
Timestamp: 2025-06-16T17:03:45.729Z
Learning: In the Ktor codebase, encoder implementations consistently use GlobalScope.writer and GlobalScope.reader patterns for compression/decompression operations. This is the established architectural pattern used throughout the codebase for byte channel transformations.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module depends on the kotlin("plugin.atomicfu") plugin, so atomicfu should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Gobley Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module requires the kotlin("plugin.atomicfu") plugin as a dependency, so the atomicfu plugin should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
🧬 Code graph analysis (2)
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (1)
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt (5)
createAndStartServer(165-197)createApacheClient(326-337)withHttp1(272-284)withHttp2(286-299)withUrl(246-260)
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt (1)
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (1)
engine(28-153)
🔇 Additional comments (13)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt (1)
84-87: LGTM with dependencies on other fixes.The
channelInactiveoverride correctly callsonConnectionCloseand propagates the event. This implementation follows the standard Netty pattern.However, ensure the issues flagged in comments on lines 29, 40-49, and 54 are resolved for this to function correctly.
ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt (1)
390-396: LGTM! Clean test class addition.The
NettyHttpRequestLifecycleTestclass properly extends the abstract test suite with Netty-specific configuration, enabling SSL and HTTP/2 to exercise the full lifecycle behavior across different protocols.ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt (1)
89-95: LGTM! Appropriate CIO test configuration.The
CIOHttpRequestLifecycleTestclass correctly disables SSL and HTTP/2, which aligns with the CIO engine's configuration in other test classes within this file.ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (4)
28-39: LGTM! Well-structured test base class.The abstract test class and
cancellableRoutehelper method provide a clean foundation for testing HTTP request lifecycle behavior across different engine implementations.
41-101: LGTM! Comprehensive cancellation test with proper resource management.The test correctly:
- Uses
client.use { }to ensure proper cleanup even on failure- Employs 10-second timeouts to handle CI load (addressing previous flakiness concerns)
- Validates that
ConnectionClosedExceptionis the root cause of cancellation- Tests across multiple protocol combinations (HTTP/1, HTTPS, HTTP/2)
The synchronization logic with atomic counters and channels is sound for concurrent test execution.
103-124: LGTM! Proper resource management in success path test.The test correctly uses
client.use { }to ensure the HttpClient is closed even if assertions fail. The test validates that the lifecycle plugin works correctly for successful requests.
126-152: LGTM! Streaming test with proper resource cleanup.The test correctly uses
client.use { }for proper resource management and validates that the lifecycle plugin works correctly with streaming responses viarespondOutputStream.ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt (6)
246-260: LGTM! Good refactoring to improve code reuse.The
withUrlmethod now delegates to the newwithHttp1helper, reducing code duplication and improving maintainability.
272-284: LGTM! Clean HTTP/1 request helper.The
withHttp1method provides a clear abstraction for HTTP/1.1 requests, properly utilizing the sharedclientinstance.
286-299: LGTM! Proper HTTP/2 client lifecycle management.The
withHttp2method correctly creates a fresh Apache client for each request and uses theuse { }pattern to ensure proper cleanup. This is appropriate for HTTP/2 testing where client configuration may differ.
303-337: LGTM! Excellent refactoring of TLS and client setup.The changes improve the test infrastructure:
keyStoreis now immutable and lazily initialized (thread-safe)createTrustManager()properly extracts the X509TrustManagercreateCIOClient()andcreateApacheClient()provide clear factory methods with appropriate TLS configuration for testing- Centralized client creation improves consistency across tests
339-343: LGTM! Proper test lifecycle management.The
setupAll()method correctly initializes the sharedclientfield using@BeforeAll, ensuring it's available for all tests. This pairs well with the existingcleanup()method that closes the client.
363-370: LGTM! Appropriate trust-all setup for testing.The
trustAllCertificatesarray is properly configured for test environments where self-signed certificates are used. The@Suppress("CustomX509TrustManager")annotation correctly acknowledges that bypassing certificate validation is intentional for testing purposes.
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt
Outdated
Show resolved
Hide resolved
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
Show resolved
Hide resolved
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
Show resolved
Hide resolved
zibet27
force-pushed
the
zibet27/request-cancellation
branch
from
be42a6b to
18b8dda
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt (1)
51-69: Consider clearingcurrentCallafter request completion.The
currentCallis assigned before launching the coroutine but is only cleared inonConnectionClose. If a request completes successfully without the connection closing,currentCallretains a reference to the completed call until the next request or connection close, potentially causing:
- Stale references being accessed if
onConnectionCloseis invoked later- Memory retention of completed calls
Consider clearing
currentCallwhen the request processing completes:private fun handleRequest(context: ChannelHandlerContext, call: PipelineCall) { val callContext = CallHandlerCoroutineName + NettyDispatcher.CurrentContext(context) currentCall = call currentJob = launch(callContext, start = CoroutineStart.UNDISPATCHED) { - when { - call is NettyHttp1ApplicationCall && !call.request.isValid() -> { - respondError400BadRequest(call) - } - - else -> - try { - enginePipeline.execute(call) - } catch (error: Throwable) { - handleFailure(call, error) - } + try { + when { + call is NettyHttp1ApplicationCall && !call.request.isValid() -> { + respondError400BadRequest(call) + } + + else -> + try { + enginePipeline.execute(call) + } catch (error: Throwable) { + handleFailure(call, error) + } + } + } finally { + if (currentCall === call) { + currentCall = null + } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.api(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt(4 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.kt(3 hunks)ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt(1 hunks)ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt(5 hunks)ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/ConfigTestSuite.kt(1 hunks)ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.kt
- ktor-server/ktor-server-core/api/ktor-server-core.api
- ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/ConfigTestSuite.kt
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: In the Ktor DI plugin, when a DependencyRegistry implements CoroutineScope, it should be properly cancelled during application shutdown to prevent memory leaks and resource issues. This is handled by calling registry.cancel() in the ApplicationStopped event.
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-11-14T14:11:30.292Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 5195
File: ktor-client/ktor-client-java/jvm/test/io/ktor/client/engine/java/JavaHttp2Test.kt:13-15
Timestamp: 2025-11-14T14:11:30.292Z
Learning: In ktor-client-java tests, Java's HttpClient does support h2c (HTTP/2 cleartext) when configured with `protocolVersion = HttpClient.Version.HTTP_2` on Java 11+. The JavaHttp2Test successfully passes with the default useH2c=true setting, connecting to http://localhost:8084.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-06-09T07:08:35.085Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4916
File: ktor-server/ktor-server-core/api/ktor-server-core.api:151-151
Timestamp: 2025-06-09T07:08:35.085Z
Learning: Breaking changes are acceptable in Ktor codebase when the code hasn't been released yet, as confirmed by bjhham from the development team.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Applied to files:
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-05-14T18:17:03.059Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: In the Ktor DI plugin, when a DependencyRegistry implements CoroutineScope, it should be properly cancelled during application shutdown to prevent memory leaks and resource issues. This is handled by calling registry.cancel() in the ApplicationStopped event.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
📚 Learning: 2025-05-14T18:17:03.059Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: When implementing CoroutineScope in Ktor components like DependencyRegistry, always ensure the scope is properly cancelled during application shutdown to prevent memory leaks and resource issues. In the DI plugin, this is handled by calling registry.cancel() in the ApplicationStopped event handler after all dependency cleanup is complete.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: JettyWebsocketConnection in ktor-server-jetty-jakarta implements Closeable interface (has close() method), so connection.use { } is valid Kotlin syntax and will compile correctly.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: The headersOf() function from io.ktor.http package is available through wildcard imports like `import io.ktor.http.*`, so no explicit import statement is needed when using wildcard imports from that package.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.ktktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-09-05T12:47:49.016Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4887
File: ktor-server/ktor-server-jetty-jakarta/jvm/src/io/ktor/server/jetty/jakarta/JettyWebsocketConnection.kt:35-52
Timestamp: 2025-09-05T12:47:49.016Z
Learning: Jetty's AbstractConnection class implements Closeable interface, so any class extending AbstractConnection (like JettyWebsocketConnection) can be used with Kotlin's use { } extension function for resource management.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt
📚 Learning: 2025-09-30T07:52:14.769Z
Learnt from: yschimke
Repo: ktorio/ktor PR: 4013
File: ktor-client/ktor-client-android/jvm/src/io/ktor/client/engine/android/Android14URLConnectionFactory.kt:24-26
Timestamp: 2025-09-30T07:52:14.769Z
Learning: In the Ktor Android HTTP client engine (ktor-client-android), prefer using `URI.create(urlString).toURL()` over the `URL(urlString)` constructor when opening connections with Android's HttpEngine, as it avoids deprecated APIs and the different exception behavior (IllegalArgumentException vs MalformedURLException) is acceptable.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-05-16T13:11:28.416Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 4860
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.api:24-26
Timestamp: 2025-05-16T13:11:28.416Z
Learning: Binary incompatible changes (such as constructor signature changes) are acceptable in the ktor-server-di module when the version is not yet released, as confirmed by the development team.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-06-16T17:03:45.729Z
Learnt from: tre3p
Repo: ktorio/ktor PR: 4936
File: ktor-utils/jvm/src/io/ktor/util/ZstdEncoding.kt:28-35
Timestamp: 2025-06-16T17:03:45.729Z
Learning: In the Ktor codebase, encoder implementations consistently use GlobalScope.writer and GlobalScope.reader patterns for compression/decompression operations. This is the established architectural pattern used throughout the codebase for byte channel transformations.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module depends on the kotlin("plugin.atomicfu") plugin, so atomicfu should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Gobley Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module requires the kotlin("plugin.atomicfu") plugin as a dependency, so the atomicfu plugin should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
🧬 Code graph analysis (3)
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (1)
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt (5)
createAndStartServer(165-197)createApacheClient(326-337)withHttp1(272-284)withHttp2(286-299)withUrl(246-260)
ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt (1)
ktor-utils/common/src/io/ktor/util/Attributes.kt (1)
AttributeKey(19-21)
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt (1)
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (1)
engine(28-153)
🪛 detekt (1.23.8)
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt
[warning] 367-367: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
[warning] 368-368: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (19)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt (1)
90-94: LGTM! Null safety properly handled.The null-safe call operator correctly handles the case where
NettyApplicationCallHandlermight not be present in the pipeline, addressing the previous concern about potentialNullPointerException.ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt (1)
390-396: LGTM! Test class follows established patterns.The new
NettyHttpRequestLifecycleTestclass properly extends the shared test suite with appropriate SSL and HTTP/2 configuration for Netty.ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt (1)
89-95: LGTM! Test class properly configured for CIO engine.The new
CIOHttpRequestLifecycleTestclass appropriately disables SSL and HTTP/2, which aligns with CIO engine capabilities.ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationCallHandler.kt (2)
40-49: LGTM! Connection close handler properly implemented.The
onConnectionClosefunction correctly checks channel state and invokes the close handler when appropriate. The early return on active channels serves as a defensive guard.
84-87: LGTM! Proper lifecycle integration.The
channelInactiveoverride correctly invokes the close handler and propagates the event through the pipeline.ktor-server/ktor-server-core/common/src/io/ktor/server/http/HttpRequestLifecycle.kt (3)
17-36: LGTM! Clear configuration with helpful documentation.The
HttpRequestLifecycleConfigclass provides a well-documented property with clear guidance on when to usecancelCallOnClose, including practical examples and use-case scenarios.
38-43: LGTM! Appropriate use of internal API.The
HttpRequestCloseHandlerKeyis correctly marked with@InternalAPIas it's an implementation detail for the lifecycle mechanism.
44-106: LGTM! Plugin implementation correctly handles edge cases.The plugin implementation properly:
- Provides comprehensive documentation with examples
- Guards against duplicate registration using
containscheck (line 94)- Only installs the handler when
cancelCallOnCloseis enabled- Cancels the call context with appropriate exception hierarchy
ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/HttpRequestLifecycleTest.kt (4)
28-39: LGTM! Clean helper method for test setup.The
cancellableRoutehelper properly installs theHttpRequestLifecycleplugin withcancelCallOnClose = trueand configures the route handler.
41-101: LGTM! Comprehensive test for client disconnection handling.The test thoroughly validates:
- Request cancellation when client disconnects
- Proper exception hierarchy (ConnectionClosedException as root cause)
- Coverage across HTTP/1.1, HTTPS, and HTTP/2 when enabled
- Proper resource cleanup with
client.useblocks- Synchronization using channels to coordinate test timing
103-124: LGTM! Test validates successful lifecycle completion.The test correctly verifies that the lifecycle handler doesn't interfere with normal request completion, ensuring
cancelCallOnCloseonly affects disconnection scenarios.
126-152: LGTM! Test validates streaming response handling.The test confirms that streaming responses work correctly with the lifecycle plugin, ensuring the handler doesn't interfere with chunked/streamed data transmission.
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/EngineTestBaseJvm.kt (7)
246-260: LGTM! Method calls updated for refactored API.The
withUrlmethod now correctly delegates towithHttp1for HTTP and HTTPS paths, aligning with the new factory-based architecture.
272-284: LGTM! Improved method naming and visibility.Renaming
withUrltowithHttp1improves clarity about the protocol version being tested, and theprotectedvisibility allows subclasses to leverage this functionality.
286-299: LGTM! Proper resource management with factory pattern.Using
createApacheClient().use { }ensures the HTTP/2 client is properly closed after each test request.
303-303: LGTM! Lazy initialization appropriate for keyStore.Changing from
lateinit vartolazy valis a cleaner approach that eliminates the need for manual initialization and ensures thread-safe initialization.
306-337: LGTM! Well-structured factory methods improve testability.The new factory methods (
createTrustManager,createCIOClient,createApacheClient) provide:
- Centralized client configuration
- Reusable SSL/TLS setup
- Better separation of concerns
- Easier customization in test subclasses
339-343: LGTM! Setup method uses factory pattern.The
setupAllmethod now uses thecreateCIOClientfactory, ensuring consistent client creation across the test suite.
363-370: LGTM! Trust-all trust manager appropriate for tests.The
trustAllCertificatesimplementation with empty methods is intentional for testing environments where certificate validation needs to be bypassed. The static analysis warnings are false positives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt (1)
92-98: CIO lifecycle test wiring looks correct and consistent with other suitesThe new
CIOHttpRequestLifecycleTestis correctly parameterized withCIOApplicationEngine/Configurationand usesCIOas the factory. Disabling SSL and HTTP/2 in theinitblock is consistent with the other CIO test suites in this file. If/when you add stable HTTP/2 coverage for CIO, it might be worth adding a parallelenableHttp2 = truevariant, but that's purely optional for this PR.ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.kt (2)
165-165: Idiomatic use of underscore for unused exception.The change correctly uses
_to indicate the caught exception is intentionally unused, following Kotlin conventions.Optionally, consider adding debug logging for reflection failures to aid in diagnosing potential Netty version compatibility issues:
- } catch (_: Throwable) { + } catch (e: Throwable) { + application.log.debug("Failed to access Netty streamKey field via reflection", e) null }
179-179: Idiomatic use of underscore for unused exception.The change correctly uses
_for the unused exception variable, making the intent clear.Consider adding debug logging to help diagnose reflection failures during HTTP/2 push promise setup:
- } catch (_: Throwable) { + } catch (e: Throwable) { + application.log.debug("Failed to invoke setStreamAndProperty via reflection", e) return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.api(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.klib.api(2 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.kt(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ktor-server/ktor-server-core/api/ktor-server-core.api
- ktor-server/ktor-server-core/api/ktor-server-core.klib.api
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/common/src/io/ktor/server/plugins/di/DependencyRegistry.kt:37-38
Timestamp: 2025-05-14T18:17:03.059Z
Learning: In the Ktor DI plugin, when a DependencyRegistry implements CoroutineScope, it should be properly cancelled during application shutdown to prevent memory leaks and resource issues. This is handled by calling registry.cancel() in the ApplicationStopped event.
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.
Applied to files:
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.kt
📚 Learning: 2025-11-14T14:11:30.292Z
Learnt from: osipxd
Repo: ktorio/ktor PR: 5195
File: ktor-client/ktor-client-java/jvm/test/io/ktor/client/engine/java/JavaHttp2Test.kt:13-15
Timestamp: 2025-11-14T14:11:30.292Z
Learning: In ktor-client-java tests, Java's HttpClient does support h2c (HTTP/2 cleartext) when configured with `protocolVersion = HttpClient.Version.HTTP_2` on Java 11+. The JavaHttp2Test successfully passes with the default useH2c=true setting, connecting to http://localhost:8084.
Applied to files:
ktor-server/ktor-server-cio/jvm/test/io/ktor/tests/server/cio/CIOEngineTestJvm.kt
🔇 Additional comments (1)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http2/NettyHttp2Handler.kt (1)
192-192: Good use of underscore for expected exception.Correctly uses
_for the unused exception variable. SinceNoSuchFieldExceptionis part of the expected control flow when traversing the class hierarchy, this change improves readability without needing additional logging.
|
lgtm! |
Subsystem
Server, Server Netty, Server CIO
Motivation
KTOR-2187 How to detect if a request was cancelled from the client on the Ktor server
Solution
Route-based plugin that sets a handler for the engines to run when the request is closed (successfully or not). The handler cancels the
call.coroutineContextwhich fits the structured concurrency model.Engines that are based on servlets don't support it. Writing a response is the only way (I found) to check if the connection is active in that case.