KTOR-8821 Fix auto-reload issue / exception on lambda #5070
Conversation
WalkthroughRefactors dynamic module discovery and error handling in EmbeddedServerJvm, adjusts Netty handlers to pass Application and include Application.coroutineContext in call coroutine contexts, changes NettyHttp2Handler signature, and removes coroutineContext from RoutingPipelineCall constructor in favor of a getter with a deprecated compatibility overload. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
| loadServiceOrNull() ?: ModuleParametersInjector.Disabled | ||
| } | ||
| private val modules by lazy { | ||
| private val modules: List<DynamicApplicationModule> get() = |
There was a problem hiding this comment.
I made this uncached, since it's only called once and this will allow for adding / removing modules at runtime.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-server/ktor-server-core/jvm/src/io/ktor/server/engine/EmbeddedServerJvm.kt (1)
426-440: Tighten wording in debug log; reference the issue in KDoc (nit)The message “auto-reloading unavailable” can be misread as globally disabled. This helper only disables name-based lookup for this module. Suggest clarifying and linking KTOR-8821.
Apply:
@@ - /** - * Method name getting might fail if method signature has been changed after compilation - * (for example by R8 or ProGuard). - * - * We must also filter out function names with $, assuming they are anonymous. - */ + /** + * Obtaining a stable method name may fail if the signature changed after compilation + * (e.g., by R8/ProGuard). Also filters out names containing '$' (assumed anonymous/generated). + * See KTOR-8821. + */ @@ - }.onFailure { cause -> - environment.log.debug("Module can't be loaded dynamically; auto-reloading unavailable", cause) + }.onFailure { cause -> + environment.log.debug( + "Dynamic lookup of programmatic module name failed; using direct invocation for this module", + cause + ) }.getOrNull()?.takeIf { '$' !in it }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ktor-server/ktor-server-core/jvm/src/io/ktor/server/engine/EmbeddedServerJvm.kt(3 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-server/ktor-server-core/jvm/src/io/ktor/server/engine/EmbeddedServerJvm.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-core/jvm/src/io/ktor/server/engine/EmbeddedServerJvm.kt
🧠 Learnings (2)
📚 Learning: 2025-05-16T13:11:28.416Z
Learnt from: osipxd
PR: ktorio/ktor#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-core/jvm/src/io/ktor/server/engine/EmbeddedServerJvm.kt
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
PR: ktorio/ktor#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-core/jvm/src/io/ktor/server/engine/EmbeddedServerJvm.kt
🔇 Additional comments (3)
ktor-server/ktor-server-core/jvm/src/io/ktor/server/engine/EmbeddedServerJvm.kt (3)
64-67: Recomputing modules per access avoids stale wrappers — LGTMThis fixes caching issues after reload and keeps behavior consistent with dynamic resolution.
399-403: Idiomatic constructor usageSwitch to the trailing-lambda constructor for DynamicApplicationModule is clean and keeps semantics unchanged.
409-424: Ensure resolution and linkage failures trigger the fallback
- Confirm that
executeModuleFunction(and by extensionlaunchModuleByName) catches and wraps any name-resolution orLinkageError(e.g.NoSuchMethodError,IncompatibleClassChangeError) inReloadingExceptionso thecatch (cause: ReloadingException)branch always fires.- Add an integration test using an anonymous module under watch that modifies a handler and verifies the server restarts without error and serves the updated behavior.
|
As it turns out, the actual breaking change was that introducing suspend modules caused the blocking function references to get wrapped during casting so that they become anonymous inner classes internally. Previously, the function reference Maybe in 3.4, we can investigate fixing this as part of the "enhanced dev mode" story, since compose hot reload works with lambdas, the solution should work here. |
bjhham
force-pushed
the
bjhham/auto-reload-fix
branch
from
51ef0c4 to
7051c7a
Compare
bjhham
changed the title
bjhham
changed the title
bjhham
force-pushed
the
bjhham/auto-reload-fix
branch
from
01d2118 to
3f5797a
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/http1/NettyHttp1Handler.kt (1)
142-150: Add a regression test for cancellation/linkage.Please add a test that:
- Cancels Application Job and asserts in-flight call is cancelled.
- Verifies call coroutineContext contains the app Job and the handler CoroutineName.
I can sketch an engine test using a custom feature to capture call coroutineContext and assert parentage—want me to draft it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.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-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.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-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.ktktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt
🧬 Code graph analysis (2)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt (1)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt (1)
applicationProvider(20-175)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt (1)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt (1)
applicationProvider(31-210)
🔇 Additional comments (5)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt (2)
115-118: Composing application.coroutineContext with userContext is correct and improves cancellation/exception propagation.Order (application + user) ensures user overrides (CoroutineName/handler) while keeping the app Job as parent. LGTM.
112-119: Verify HTTP/2 hot-reload behaviorHTTP/2 connections now capture a single Application instance per connection (NettyHttp2Handler’s constructor takes a concrete Application, not a provider). I ran ripgrep across ktor-server-netty and confirmed no per-stream provider lookups remain. Please verify that hot-reloading on long-lived HTTP/2 sessions doesn’t continue routing to the pre-reload Application—run a reload test against an active H2 client.
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.kt (2)
208-211: Good: avoid capturing Application coroutineContext at bootstrap time.Building userContext without applicationProvider().coroutineContext prevents stale context leaks across reloads and aligns with composing at the call site.
208-211: The above script will locate and display howuserContext,application.coroutineContext, and coroutine launches are set up in the Netty handler. Please share its output so we can confirm whether all call paths correctly compose the application context.ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt (1)
142-150: Per-request Application retrieval + context composition looks right.Fetching application once per request and using application.coroutineContext + userContext should restore correct parentage and exception handling for calls. Nice cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt (2)
112-119: HTTP/2: Capturing Application at channel init can pin stale instance across reloads.Evaluating
applicationProvider()once and passingapplication+ its context to the HTTP/2 handler ties the channel to that instance. On long-lived HTTP/2 connections, new streams after a reload may still use the old Application and keep it from being GC’d. Please verify reload behavior with a persistent HTTP/2 connection and consider deferring resolution per-stream (or ensuring channels are closed on reload).If feasible with NettyHttp2Handler’s API, prefer passing the provider and composing the context lazily inside the handler:
- val application = applicationProvider() - val handler = NettyHttp2Handler( - enginePipeline, - application, - callEventGroup, - application.coroutineContext + userContext, - runningLimit - ) + val handler = NettyHttp2Handler( + enginePipeline, + applicationProvider, // resolve per-stream inside the handler + callEventGroup, + userContext, // compose: applicationProvider().coroutineContext + userContext + runningLimit + )
117-117: CoroutineContext composition: guard against overriding the Application Job.
a + blets elements inboverridea. IfuserContextever carries a Job, it will overshadowapplication.coroutineContext’s Job. Defensive tweak:- application.coroutineContext + userContext, + application.coroutineContext + userContext.minusKey(Job),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.kt
- ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt
🧰 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-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.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-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt
🧬 Code graph analysis (1)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt (1)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt (1)
applicationProvider(20-175)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt (1)
507-511: Tiny simplification: inline the null-check.Keeps the assert message and removes a temp variable.
- val testDataFromParent = call.coroutineContext[TestData] - assertNotNull(testDataFromParent, "Context should contain test data from parent") - call.respond(HttpStatusCode.OK, testDataFromParent.name) + val name = requireNotNull(call.coroutineContext[TestData]) { + "Context should contain test data from parent" + }.name + call.respond(HttpStatusCode.OK, name)ktor-server/ktor-server-core/api/ktor-server-core.api (1)
1789-1790: Deprecate the 6-arg RoutingPipelineCall constructor
Annotate the overload taking(PipelineCall, RoutingNode, CoroutineContext, ApplicationReceivePipeline, ApplicationSendPipeline, Parameters)in RoutingPipelineCall.kt with@Deprecated( "Pass coroutineContext via PipelineCall—use the 5-arg constructor", ReplaceWith( "RoutingPipelineCall(pipelineCall, node, receivePipeline, sendPipeline, parameters)" ) )and add a KDoc migration note pointing users to the 5-arg overload.
ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt (3)
30-44: Deprecated ctor ignores coroutineContext — add ReplaceWith and suppress unused paramThe deprecation message is good, but the unused coroutineContext param will warn and IDE quick-fix is missing. Add ReplaceWith and suppress the unused parameter.
Apply:
- @Deprecated(level = DeprecationLevel.WARNING, message = "CoroutineContext is delegated to engineCall now.") + @Deprecated( + level = DeprecationLevel.WARNING, + message = "CoroutineContext is delegated to engineCall now.", + replaceWith = ReplaceWith( + "RoutingPipelineCall(engineCall, route, receivePipeline, responsePipeline, pathParameters)" + ) + ) + @Suppress("UNUSED_PARAMETER") public constructor( engineCall: PipelineCall, route: RoutingNode, coroutineContext: CoroutineContext, receivePipeline: ApplicationReceivePipeline, responsePipeline: ApplicationSendPipeline, pathParameters: Parameters ) : this( engineCall, route, receivePipeline, responsePipeline, pathParameters )
15-22: KDoc property name is stale (call → engineCall)KDoc still documents “@Property call …” but the property is engineCall. Please update to avoid misleading IDE tooltips.
- * @property call original call from [io.ktor.server.engine.ApplicationEngine] + * @property engineCall original call from [io.ktor.server.engine.ApplicationEngine] * @property route is the selected route
63-63: toString label mismatchtoString() returns “RoutingApplicationCall(...)” while the class is RoutingPipelineCall. Consider aligning for clearer logs.
- override fun toString(): String = "RoutingApplicationCall(route=$route)" + override fun toString(): String = "RoutingPipelineCall(route=$route)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ktor-server/ktor-server-core/api/ktor-server-core.api(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.klib.api(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingRoot.kt(0 hunks)ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt(1 hunks)ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt(1 hunks)
💤 Files with no reviewable changes (1)
- ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingRoot.kt
✅ Files skipped from review due to trivial changes (1)
- ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.ktktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.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/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.ktktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt
**/api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Track all public API changes in the /api/ directories
Files:
ktor-server/ktor-server-core/api/ktor-server-core.apiktor-server/ktor-server-core/api/ktor-server-core.klib.api
🧬 Code graph analysis (1)
ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt (4)
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/BaseTestJvm.kt (1)
runTest(49-63)ktor-server/ktor-server-test-base/common/src/io/ktor/server/test/base/BaseTest.kt (1)
runTest(20-24)ktor-server/ktor-server-test-base/posix/src/io/ktor/server/test/base/EngineTestBaseNix.kt (1)
createAndStartServer(65-86)ktor-server/ktor-server-test-base/jsAndWasmShared/src/io/ktor/server/test/base/EngineTestBase.jsAndWasmShared.kt (1)
createAndStartServer(71-91)
🔇 Additional comments (5)
ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt (2)
503-503: Making the test overridable is appropriate.Allows engine-specific overrides without duplicating the common suite.
508-508: Correct context source: use call.coroutineContext.This validates propagation from the parent into the Application/Call scope, matching the engine changes.
ktor-server/ktor-server-core/api/ktor-server-core.api (1)
1788-1788: Additive RoutingPipelineCall constructor looks good.Binary-safe; aligns with delegating coroutineContext to engineCall.
ktor-server/ktor-server-core/api/ktor-server-core.klib.api (1)
1109-1111: Public API: new RoutingPipelineCall overload — add “Since” note and verify all API dumps
- Add KDoc on the 5-arg constructor with a “Since: ” tag.
- Retain the deprecated 6-arg constructor until the next major release.
- Regenerate all API dumps; the klib.api now lists both ctors, but no .jar.api files were found—please confirm your JAR API dump includes the new overload.
ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt (1)
48-48: Ignore context delegation concern DelegatingcoroutineContexttoengineCalldoesn’t change behavior because theRoutingPipelineCallconstructor has noCoroutineContextparameter—call sites can’t pass a custom context.Likely an incorrect or invalid review comment.
bjhham
force-pushed
the
bjhham/auto-reload-fix
branch
from
53935ee to
c44c677
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt (1)
801-805: Fix incorrect KDoc reference for the context KeyThe KDoc says “Key for [CoroutineName]” but this is the key for TestData.
Apply:
- /** - * Key for [CoroutineName] instance in the coroutine context. - */ + /** + * Key for [TestData] instance in the coroutine context. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ktor-server/ktor-server-core/api/ktor-server-core.api(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.klib.api(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingRoot.kt(0 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt(2 hunks)ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt(1 hunks)ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt(1 hunks)
💤 Files with no reviewable changes (1)
- ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingRoot.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt
- ktor-server/ktor-server-core/api/ktor-server-core.klib.api
- ktor-server/ktor-server-core/api/ktor-server-core.api
- ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt
- ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt
- ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt
🧰 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-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.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/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt
🧬 Code graph analysis (1)
ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt (4)
ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/BaseTestJvm.kt (1)
runTest(49-63)ktor-server/ktor-server-test-base/common/src/io/ktor/server/test/base/BaseTest.kt (1)
runTest(20-24)ktor-server/ktor-server-test-base/posix/src/io/ktor/server/test/base/EngineTestBaseNix.kt (1)
createAndStartServer(65-86)ktor-server/ktor-server-test-base/jsAndWasmShared/src/io/ktor/server/test/base/EngineTestBase.jsAndWasmShared.kt (1)
createAndStartServer(71-91)
🔇 Additional comments (2)
ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt (2)
503-503: Making the test overridable is appropriateOpening this test enables engine-specific overrides in downstream suites without duplicating logic. LGTM.
508-511: No legacy kotlin.coroutines.coroutineContext remains in HTTP server handlers/tests – all found imports are in client-side and test-host modules, where using the top-levelcoroutineContextis intentional.
bjhham
force-pushed
the
bjhham/auto-reload-fix
branch
from
c44c677 to
b7b7e06
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt (2)
112-119: Verify autoreload behavior for long‑lived HTTP/2 connectionsBecause
applicationProvider()is evaluated once per connection during pipeline setup, an open HTTP/2 connection will keep using thatApplicationafter a reload. If dev-mode should reflect changes without requiring clients to reconnect, consider:
- sending GOAWAY/closing H2 channels on reload, or
- letting the H2 handler resolve the app per-stream (larger change).
Please confirm current expected behavior and whether channel teardown on reload is handled elsewhere.
117-117: Context merge order: ensure userContext never carries a Job
application.coroutineContext + userContextlets right-most elements override; ifuserContextever contains aJob, it would override theApplicationjob and break cancellation semantics on reload/shutdown. Please confirmuserContextnever includes aJob. If that’s not guaranteed, flip the order.- application.coroutineContext + userContext, + userContext + application.coroutineContext,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ktor-server/ktor-server-core/api/ktor-server-core.api(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.klib.api(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingRoot.kt(0 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt(2 hunks)ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt(1 hunks)ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt(1 hunks)
💤 Files with no reviewable changes (1)
- ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingRoot.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettyEngineTest.kt
- ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt
- ktor-server/ktor-server-core/api/ktor-server-core.klib.api
- ktor-server/ktor-server-core/api/ktor-server-core.api
- ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt
- ktor-server/ktor-server-test-suites/common/src/io/ktor/server/testing/suites/HttpServerCommonTestSuite.kt
🧰 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-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.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-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt
🧬 Code graph analysis (1)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt (1)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt (1)
applicationProvider(20-179)
🔇 Additional comments (1)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt (1)
112-119: Propagating the current Application and coroutineContext into HTTP/2 handler looks correctPassing
applicationandapplication.coroutineContext + userContextintoNettyHttp2Handleraligns with the PR goal to avoid stale context during reloads. Wiring/order of args matches the new ctor.
bjhham
force-pushed
the
bjhham/auto-reload-fix
branch
2 times, most recently
from
a522259 to
9bd531b
Compare
bjhham
force-pushed
the
bjhham/auto-reload-fix
branch
from
9bd531b to
21cf951
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ktor-server/ktor-server-core/api/ktor-server-core.api(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.klib.api(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt(1 hunks)ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingRoot.kt(0 hunks)ktor-server/ktor-server-core/jvm/src/io/ktor/server/engine/EmbeddedServerJvm.kt(3 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt(1 hunks)ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt(2 hunks)
💤 Files with no reviewable changes (1)
- ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingRoot.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt
- ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.kt
- ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/http1/NettyHttp1Handler.kt
- ktor-server/ktor-server-core/api/ktor-server-core.api
- ktor-server/ktor-server-core/jvm/src/io/ktor/server/engine/EmbeddedServerJvm.kt
- ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyChannelInitializer.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Track all public API changes in the /api/ directories
Files:
ktor-server/ktor-server-core/api/ktor-server-core.klib.api
| constructor <init>(io.ktor.server.application/PipelineCall, io.ktor.server.routing/RoutingNode, io.ktor.server.request/ApplicationReceivePipeline, io.ktor.server.response/ApplicationSendPipeline, io.ktor.http/Parameters) // io.ktor.server.routing/RoutingPipelineCall.<init>|<init>(io.ktor.server.application.PipelineCall;io.ktor.server.routing.RoutingNode;io.ktor.server.request.ApplicationReceivePipeline;io.ktor.server.response.ApplicationSendPipeline;io.ktor.http.Parameters){}[0] | ||
| constructor <init>(io.ktor.server.application/PipelineCall, io.ktor.server.routing/RoutingNode, kotlin.coroutines/CoroutineContext, io.ktor.server.request/ApplicationReceivePipeline, io.ktor.server.response/ApplicationSendPipeline, io.ktor.http/Parameters) // io.ktor.server.routing/RoutingPipelineCall.<init>|<init>(io.ktor.server.application.PipelineCall;io.ktor.server.routing.RoutingNode;kotlin.coroutines.CoroutineContext;io.ktor.server.request.ApplicationReceivePipeline;io.ktor.server.response.ApplicationSendPipeline;io.ktor.http.Parameters){}[0] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Public API change to RoutingPipelineCall constructor looks good; please deprecate the old overload and document migration.
- Adding the 5-arg constructor (without CoroutineContext) is source/binary-safe and aligns with routing calls inheriting the engineCall's context.
- Keep the 6-arg overload, but mark it @deprecated with a ReplaceWith that calls the new constructor, and ensure the CoroutineContext parameter is ignored. Add KDoc noting coroutineContext now always delegates to engineCall.coroutineContext.
Suggested Kotlin change (in RoutingPipelineCall.kt):
class RoutingPipelineCall(
engineCall: PipelineCall,
route: RoutingNode,
receivePipeline: ApplicationReceivePipeline,
responsePipeline: ApplicationSendPipeline,
parameters: Parameters
) : PipelineCall, CoroutineScope {
- // ensure this delegates to engineCall
+ // Always reflects the current engineCall context
override val coroutineContext: CoroutineContext
get() = engineCall.coroutineContext
@@
- constructor(
- engineCall: PipelineCall,
- route: RoutingNode,
- coroutineContext: CoroutineContext,
- receivePipeline: ApplicationReceivePipeline,
- responsePipeline: ApplicationSendPipeline,
- parameters: Parameters
- )
+ @Deprecated(
+ message = "RoutingPipelineCall now derives coroutineContext from engineCall. Use the constructor without CoroutineContext.",
+ replaceWith = ReplaceWith(
+ "RoutingPipelineCall(engineCall, route, receivePipeline, responsePipeline, parameters)"
+ ),
+ level = DeprecationLevel.WARNING
+ )
+ constructor(
+ engineCall: PipelineCall,
+ route: RoutingNode,
+ coroutineContext: CoroutineContext,
+ receivePipeline: ApplicationReceivePipeline,
+ responsePipeline: ApplicationSendPipeline,
+ parameters: Parameters
+ ) : this(engineCall, route, receivePipeline, responsePipeline, parameters)Run to verify deprecation and delegation are present and no internal callers rely on the old param:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify coroutineContext getter delegates to engineCall
rg -n --type=kotlin -C2 $'override\\s+val\\s+coroutineContext\\s*:\\s*CoroutineContext\\s*\\n\\s*get\\(\\)\\s*=\\s*engineCall\\.coroutineContext' ktor-server/ktor-server-core/common/src
# Verify the old constructor is deprecated
rg -n --type=kotlin -C2 $'@Deprecated\\(.*\\)\\s*constructor\\s*\\([^)]*CoroutineContext' ktor-server/ktor-server-core/common/src
# Spot any remaining named-arg usages of the old overload
rg -n --type=kotlin -C2 $'RoutingPipelineCall\\s*\\([^)]*coroutineContext\\s*='Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1. Check that coroutineContext getter is present and delegates to engineCall
rg -n --type kotlin 'override\s+val\s+coroutineContext' ktor-server/ktor-server-core/common/src
rg -n --type kotlin 'get\(\)\s*=\s*engineCall\.coroutineContext' ktor-server/ktor-server-core/common/src
# 2. Check that the old constructor is marked @Deprecated
rg -n --type kotlin '@Deprecated' ktor-server/ktor-server-core/common/src | grep -C1 'constructor'
# 3. Spot any remaining named-arg usages of the old overload
rg -n --type kotlin 'RoutingPipelineCall\([^)]*coroutineContext\s*=' .Length of output: 667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List all constructor declarations in RoutingPipelineCall.kt to verify overloads and deprecation
rg -n --type kotlin -C3 'constructor' ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.ktLength of output: 339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Display class declaration and its constructors with context
rg -n --type kotlin 'class RoutingPipelineCall' -C5 ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt
rg -n --type kotlin 'constructor' -C3 ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.ktLength of output: 846
Add ReplaceWith to deprecated constructor and update class KDoc
- In
ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt, update the@Deprecatedon the 6-arg secondary constructor (around line 30) to include
replaceWith = ReplaceWith("RoutingPipelineCall(engineCall, route, receivePipeline, responsePipeline, parameters)"). - In the KDoc above
class RoutingPipelineCall(lines 18–22), add
@property coroutineContext now always delegates to engineCall.coroutineContext.
🤖 Prompt for AI Agents
In ktor-server-core/common/src/io/ktor/server/routing/RoutingPipelineCall.kt
around lines 18–30, update the class KDoc (lines 18–22) to add the sentence
"@property coroutineContext now always delegates to
engineCall.coroutineContext", and modify the @Deprecated annotation on the
6-argument secondary constructor (around line 30) to include replaceWith =
ReplaceWith("RoutingPipelineCall(engineCall, route, receivePipeline,
responsePipeline, parameters)") so the deprecated constructor points to the
5-arg replacement.
Subsystem
Server, Core
Motivation
KTOR-8821 Autoreloading: it doesn't reload rebuilt classes since 3.2.0
KTOR-8810 Autoreloading: JobCancellationException when app is reloaded in the debugger since 3.2.0
Solution
Fix 1: dynamic module wrapping for lambdas would cause errors on reloads
Fix 2: Netty was using the old application instance in the coroutine context, which caused auto-reloading to fail when debugging.
This also fixes the issue with the static resources.