KTOR-7760 Clean up resources when bind fails with non-BindException#5587
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNettyApplicationEngine.start(wait) now catches any Throwable during channel binding and calls terminate() to ensure resource cleanup before rethrowing. A new test verifies that non-BindException failures also trigger ExecutorService termination. ChangesResource Cleanup on Non-BindException Failures
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettySpecificTest.kt (1)
76-79: ⚡ Quick winAssertion only checks the boss group — child group termination is not verified
it.config().group()returns only theconnectionEventGroup(boss group). Sincestop(0, 0)also shuts downworkerEventGroup(child group), verifying both would give stronger coverage and align with the motivation for usingstop()overterminate()(which omitsworkerEventGroup).✅ Proposed assertion expansion
assertTrue( - server.engine.bootstraps.all { (it.config().group() as ExecutorService).isTerminated }, + server.engine.bootstraps.all { + (it.config().group() as ExecutorService).isTerminated && + (it.config().childGroup() as ExecutorService).isTerminated + }, "event loop groups must be terminated when bind fails with a non-BindException" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettySpecificTest.kt` around lines 76 - 79, The current assertion only checks the boss (connectionEventGroup) via server.engine.bootstraps and it.config().group(); update the assertion to verify both boss and worker (child) event loop groups are terminated when bind fails: for each bootstrap in server.engine.bootstraps, check that (it.config().group() as ExecutorService).isTerminated AND the child/worker event group (access the bootstrap's child group, e.g. it.config().childGroup() or equivalent API that returns the workerEventGroup) is also terminated, and include a clear assertion message about verifying both groups.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.kt`:
- Around line 260-266: The catch-all startup handler currently calls stop(0, 0)
which fires ApplicationStopPreparing and may mask the original exception; change
the catch (Throwable) path in start() to call terminate() instead of stop(0, 0)
and update terminate() to also shut down workerEventGroup (in addition to
bossGroup/workerGroup) so terminate() performs the full, silent shutdown used on
BindException; ensure both the BindException and generic Throwable branches call
terminate() so lifecycle events are not raised during failed startup.
---
Nitpick comments:
In
`@ktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettySpecificTest.kt`:
- Around line 76-79: The current assertion only checks the boss
(connectionEventGroup) via server.engine.bootstraps and it.config().group();
update the assertion to verify both boss and worker (child) event loop groups
are terminated when bind fails: for each bootstrap in server.engine.bootstraps,
check that (it.config().group() as ExecutorService).isTerminated AND the
child/worker event group (access the bootstrap's child group, e.g.
it.config().childGroup() or equivalent API that returns the workerEventGroup) is
also terminated, and include a clear assertion message about verifying both
groups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae2cc19f-2b88-434a-88ad-a78526f3a347
📒 Files selected for processing (2)
ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.ktktor-server/ktor-server-netty/jvm/test/io/ktor/tests/server/netty/NettySpecificTest.kt
| } catch (cause: BindException) { | ||
| terminate() | ||
| throw cause | ||
| } catch (cause: Throwable) { | ||
| stop(0, 0) | ||
| throw cause | ||
| } |
There was a problem hiding this comment.
stop(0, 0) fires lifecycle events on a never-started server and can mask the root cause
Two problems with using stop(0, 0) in the new catch (Throwable) path instead of terminate():
-
Lifecycle event misfiring:
stop()unconditionally callsmonitor.raise(ApplicationStopPreparing, environment).ApplicationStopPreparingis semantically meaningful only for a running server. Firing it during a failed startup means any user-registered listener receives this event even thoughApplicationStarted/ServerReadywas never raised, potentially dereferencing components that were never initialized. -
Original exception can be masked:
monitor.raise()insidestop()is not guarded bywithStopException. If a listener throws, that exception propagates out ofstop()and thethrow causeon line 265 is never reached — the original bind failure is lost.
The BindException path uses terminate() which avoids both problems (no lifecycle events, targeted shutdown). The simplest consistent fix is to reuse terminate() here, while also addressing the pre-existing gap in terminate() — it currently omits workerEventGroup. A clean resolution would be to add workerEventGroup shutdown to terminate() and use it in both branches:
🛠️ Proposed fix
private fun terminate() {
withStopException {
connectionEventGroup.shutdownGracefully().sync()
}
+ withStopException {
+ workerEventGroup.shutdownGracefully().sync()
+ }
withStopException {
callEventGroup.shutdownGracefully().sync()
}
}Then in start():
- } catch (cause: Throwable) {
- stop(0, 0)
- throw cause
- }
+ } catch (cause: Throwable) {
+ terminate()
+ throw cause
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (cause: BindException) { | |
| terminate() | |
| throw cause | |
| } catch (cause: Throwable) { | |
| stop(0, 0) | |
| throw cause | |
| } | |
| } catch (cause: BindException) { | |
| terminate() | |
| throw cause | |
| } catch (cause: Throwable) { | |
| terminate() | |
| throw cause | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.kt`
around lines 260 - 266, The catch-all startup handler currently calls stop(0, 0)
which fires ApplicationStopPreparing and may mask the original exception; change
the catch (Throwable) path in start() to call terminate() instead of stop(0, 0)
and update terminate() to also shut down workerEventGroup (in addition to
bossGroup/workerGroup) so terminate() performs the full, silent shutdown used on
BindException; ensure both the BindException and generic Throwable branches call
terminate() so lifecycle events are not raised during failed startup.
There was a problem hiding this comment.
This seems correct @fru1tworld - we can just generalize the catch(cause: BindException) to catch(cause: Throwable) for all startup failures.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
bjhham
left a comment
bjhham
left a comment
There was a problem hiding this comment.
Looks like terminate would be more appropriate here.
| } catch (cause: BindException) { | ||
| terminate() | ||
| throw cause | ||
| } catch (cause: Throwable) { | ||
| stop(0, 0) | ||
| throw cause | ||
| } |
There was a problem hiding this comment.
This seems correct @fru1tworld - we can just generalize the catch(cause: BindException) to catch(cause: Throwable) for all startup failures.
|
Thanks for the review! Merged the catch blocks. 😄 |
2 participants
Subsystem
Server, Netty
Motivation
KTOR-7760 Server finishes working without an exception when no privileges to open the port
NettyApplicationEngine.start()only catchesBindException. Other bind failures (SocketExceptionfrom Linux NIO,Errors.NativeIoExceptionfrom epoll,IllegalArgumentExceptionfor invalid ports) propagate uncaught while the already-initialized Netty event loop groups stay alive on non-daemon threads, so the process hangs or appears to exit silently instead of surfacing the error.Solution
Add a
catch (Throwable)branch that callsstop(0, 0)before rethrowing, mirroring the existingBindExceptioncleanup path. New test (start cleans up resources on non-BindException failure) triggers an out-of-range port to assert the event loop groups are terminated after a non-BindExceptionfailure.