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 #5494

Closed
Lincong opened this issue Mar 10, 2024 · 1 comment · Fixed by #5524
Closed

Avoid addShutdownHook to JVM during its shutdown #5494

Lincong opened this issue Mar 10, 2024 · 1 comment · Fixed by #5524
Labels

Comments

@Lincong
Copy link

Lincong commented Mar 10, 2024

Discussion context.

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

More specifically, one situation can be that a client (e.g. Armeria gRPC client) was created for the first time leads to DefaultClientFactory being used for the first time. The JVM is already in shutdown when that happens.

A simple improvement could be:

static {
    if (DefaultClientFactory.class.getClassLoader() == ClassLoader.getSystemClassLoader()) {
      if (!shutdownHookDisabled) { // Skip adding the hook if we explicitly disabled it.
        Runtime.getRuntime().addShutdownHook(new Thread(() -> {
          ClientFactory.closeDefault();
        }));
      }
    }
}

and/or try-catch the java.lang.IllegalStateException: Shutdown in progress exception around Runtime.getRuntime().addShutdownHook and log something like "Trying to add shutdown hook during JVM shutdown, skip it because it does not matter any more since the JVM is already shutting down :)".

@thachlp
Copy link
Member

thachlp commented Mar 20, 2024

Just create a pr as @Lincong suggest.
@ikhoon please help review

jrhee17 added a commit that referenced this issue Apr 2, 2024
Motivation:

Currently a JVM shutdown hook is added when DefaultClientFactory is used
for the first time
([code](https://github.com/line/armeria/blob/main/core/src/main/java/com/linecorp/armeria/client/DefaultClientFactory.java#L80-L83)).
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:
- Closes #5494

---------

Co-authored-by: jrhee17 <guins_j@guins.org>
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 a pull request may close this issue.

3 participants