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

Avoid addShutdownHook to JVM during its shutdown #5524

Merged

Conversation

thachlp
Copy link
Member

@thachlp thachlp commented Mar 20, 2024

Motivation:

Currently a JVM shutdown hook is added when DefaultClientFactory is used for the first time (code). It is possible that a JVM shutdown hook is being added after the JVM has started its shutdown and results in an exception below:

Caused by: java.lang.IllegalStateException: Shutdown in progress
    at java.base/java.lang.ApplicationShutdownHooks.add(ApplicationShutdownHooks.java:66)
    at java.base/java.lang.Runtime.addShutdownHook(Runtime.java:216)
    at grpc_shaded.com.linecorp.armeria.client.DefaultClientFactory.<clinit>(DefaultClientFactory.java:79)
    ... 24 more

Modifications:

  • Skip adding the hook if we explicitly disabled it.
  • Add try-catch for java.lang.IllegalStateException

Result:

@ikhoon ikhoon added the defect label Mar 25, 2024
@ikhoon ikhoon added this to the 1.28.0 milestone Mar 25, 2024
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, @thachlp! 👍

Runtime.getRuntime().addShutdownHook(new Thread(() -> {
if (!shutdownHookDisabled) {
ClientFactory.closeDefault();
if (!shutdownHookDisabled) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't do anything because the static initialization block is always called before static method(e.g. disableShutdownHook0() here).
Let's just catch the exception instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for explain, updated 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't do anything because the static initialization block is always called before static method(e.g. disableShutdownHook0() here).

Question) Would users still want to disable automatic shutdown to a later timing of their choosing by calling ClientFactory#disableShutdownHook? or is #1472 not an issue anymore?

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.

Looks good. Thanks!

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Added some minor commits - let me know if any of the changes doesn't make sense. Thanks @thachlp ! 🙇 👍 🙇

@thachlp
Copy link
Member Author

thachlp commented Apr 2, 2024

Added some minor commits - let me know if any of the changes doesn't make sense. Thanks @thachlp ! 🙇 👍 🙇

Thanks @jrhee17🙇

@jrhee17 jrhee17 merged commit 98d460c into line:main Apr 2, 2024
16 checks passed
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.

Avoid addShutdownHook to JVM during its shutdown
4 participants