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

Make sure boss EventLoopGroups are terminated on startup fail… #2288

Merged
merged 5 commits into from Dec 3, 2019

Conversation

trustin
Copy link
Member

@trustin trustin commented Dec 2, 2019

Related: https://stackoverflow.com/q/58891320/55808
Motivation:

When a server fails to start up, its boss EventLoopGroup is not
terminated, leaving a dangling non-daemon thread which prevents a JVM
process from terminating itself.

Modifications:

  • Make sure the boss EventLoopGroups are always terminated by keeping
    the boss groups created by Server.

Result:

  • No more dangling non-daemon threads on server startup failure

Related: https://stackoverflow.com/q/58891320/55808
Motivation:

When a server fails to start up, its boss `EventLoopGroup` is not
terminated, leaving a dangling non-daemon thread which prevents a JVM
process from terminating itself.

Modifications:

- Make sure the boss `EventLoopGroup`s are always terminated by keeping
  the boss groups created by `Server`.

Result:

- No more dangling non-daemon threads on server startup failure
@trustin trustin added the defect label Dec 2, 2019
@trustin trustin added this to the 0.97.0 milestone Dec 2, 2019
@codecov
Copy link

codecov bot commented Dec 2, 2019

Codecov Report

Merging #2288 into master will decrease coverage by <.01%.
The diff coverage is 92.3%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #2288      +/-   ##
===========================================
- Coverage     73.71%   73.7%   -0.01%     
- Complexity     9897    9900       +3     
===========================================
  Files           871     871              
  Lines         37981   37982       +1     
  Branches       4655    4655              
===========================================
  Hits          27996   27996              
+ Misses         7607    7606       -1     
- Partials       2378    2380       +2
Impacted Files Coverage Δ Complexity Δ
.../main/java/com/linecorp/armeria/server/Server.java 84.12% <92.3%> (-1.14%) 28 <1> (-1)
...inecorp/armeria/server/grpc/ArmeriaServerCall.java 87.1% <0%> (-1.18%) 85% <0%> (ø)
...linecorp/armeria/client/HttpRequestSubscriber.java 72.32% <0%> (+0.62%) 35% <0%> (+1%) ⬆️
.../linecorp/armeria/client/Http2ResponseDecoder.java 64.16% <0%> (+0.83%) 34% <0%> (+1%) ⬆️
...om/linecorp/armeria/client/HttpSessionHandler.java 72.03% <0%> (+2.54%) 33% <0%> (+2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa64d66...ca40e6e. Read the comment docs.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Co-Authored-By: Anuraag Agrawal <anuraaga@gmail.com>
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@trustin trustin changed the title Make sure boss EventLoopGroups are terminated on startup failure Make sure boss EventLoopGroups are terminated on startup fail… Dec 3, 2019
@trustin trustin merged commit 558dc04 into line:master Dec 3, 2019
@trustin trustin deleted the fix_unclean_shutdown branch December 3, 2019 07:11
@trustin
Copy link
Member Author

trustin commented Dec 3, 2019

Thanks for reviewing!

@trustin
Copy link
Member Author

trustin commented Dec 8, 2019

Special thanks to @magnuschatt for reporting this issue via StackOverflow - https://stackoverflow.com/questions/58891320/how-to-make-armeria-exit-on-an-address-already-in-use-error

fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
…e#2288)

Related: https://stackoverflow.com/q/58891320/55808
Motivation:

When a server fails to start up, its boss `EventLoopGroup` is not
terminated, leaving a dangling non-daemon thread which prevents a JVM
process from terminating itself.

Modifications:

- Make sure the boss `EventLoopGroup`s are always terminated by keeping
  the boss groups created by `Server`.

Result:

- No more dangling non-daemon threads on server startup failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants