Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graceful Netty server shutdown in case of startup errors #3558

Merged
merged 23 commits into from
Mar 20, 2024

Conversation

goawash
Copy link
Contributor

@goawash goawash commented Mar 1, 2024

In case of an error during the start of Netty, e.g., when trying to bind to the same port, the Netty server doesn't release all resources. Due to that, the app's main thread keeps working and cannot gracefully shut down the process entirely. This happens because no resource cleanup is executed, and Event Loop is still working after Netty couldn't be started.
The bug is straightforward to reproduce:

  1. Run a simple app with Netty Server.
  2. Run the same app on the same port. There should be an exception thrown:
    Exception during binding io.netty.channel.unix.Errors$NativeIoException: bind(..) failed: Address already in use
  3. The second instance of the app keeps working, though Netty couldn't start.

The motivation of this PR is to release allocated resources in case of a failed start of the Netty Server. I have done this fix for all Netty-based servers.

@goawash goawash force-pushed the netty-binding-error-shutdown branch from ff5b884 to 035cda1 Compare March 4, 2024 10:21
@kciesielski
Copy link
Member

Thanks! It would be good to include similar improvement for other netty servers (cats, zio, loom). Maybe you could add them to this PR?

@goawash
Copy link
Contributor Author

goawash commented Mar 4, 2024

Hey @kciesielski, sure, I will try to update those you mentioned as well

@goawash
Copy link
Contributor Author

goawash commented Mar 13, 2024

Hey @kciesielski! I added support for other Netty-based servers. However, please keep in mind that I don't have too much experience with either Cats or Zio, so I hope I have fixed it correctly there🙂

@kciesielski
Copy link
Member

@goawash Thanks a lot for this contribution! I just added a small update to NettyIdServer to hook the stopRecovering logic, happy to merge the PR now.

@kciesielski kciesielski merged commit 973c1b3 into softwaremill:master Mar 20, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants