spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.66k stars 40.57k forks source link

If the JVM is killed while refresh is in progress, the shutdown hook does not close the context #23625

Closed StoffelCPR closed 3 years ago

StoffelCPR commented 3 years ago

Heyo,

Problem

We currently have an Issue with spring-boot and liquibase.

We are using the integrated liquibase to manage our databases and we are using Chef to deploy. After having databaseloglock issue we found out that our deployment process triggers a SIGTERM (Graceful shutdown) but the application shuts down immediately, like a kill, without exiting liquibase properly.

That liquibase has itself no proper shutdown/error mechanism is their problem and is under discussion in the following issue: https://github.com/liquibase/liquibase/issues/1453

But that liquibase is running without the spring-applications shutdown hook is another thing.

Expected behavior

When I use spring-boot and the integrated liquibase I'd expect that spring manages everything, including liquibase.

Spring does register a ShutdownHook after the initalization is completed but liquibase runs during startup.

There should be a shutdown hook first and that shutdown hook should trigger a liquibase shutdown or wait for spring to successfully initialize before commencing the shutdown.

wilkinsona commented 3 years ago

We currently register the shutdown hook after the application context has been refreshed. We can consider registering the shutdown hook before the context is refreshed, but it's not clear to me that it will solve your problem as I don't know its exact details.

You could try it yourself using an ApplicationContextInitializer:

public static void main(String[] args) {
    new SpringApplicationBuilder(ExampleApplication.class)
        .initializers((context) -> context.registerShutdownHook()).run(args);
}

Alternatively, could you problem a minimal sample that reproduces or at least simulates the problem so that we can analyse the behaviour? You can share a sample with us by zipping it up and attaching it to this issue or by pushing it to a separate repository on GitHub.

StoffelCPR commented 3 years ago

I'll prepare an example later today.. In short tho:

Liquibase gets executed without spring managing the run.

If there's a premature shutdown of the application liquibase just dies and creates an error.

I can try if the behaviour changes with your provided code.

But it would be nice if liquibase doesn't run loose next to spring.

Another example:

I just had a case where the application stopped during start-up due to missing properties and cyclic dependencies. Liquibase still started and without my workaround would have cause a database lock.

StoffelCPR commented 3 years ago

Your provided code does change the behavior as intended.

I haven't tested it completely yet since there's no log output.. The application just waits until liquibase is finished it seems.

philwebb commented 3 years ago

See #4053 for some more background

StoffelCPR commented 3 years ago

theoretically yes but since you can't just shutdown liquibase this is a seperate issue.

Liquibase is currently working on a default shutdown behavior.

bclozel commented 3 years ago

@stoffel2107 we know it's a separate issue, we've just discussed this issue during a team meeting and we were tracking related changes/particular reasons with this is behaving like this right now (basically trying to avoid creating unrelated regressions by fixing this).

This issue is not closed as a duplicate, this is merely a way for us to link issues.

notjustacoder commented 3 years ago

I've come across a side of effect of this fix. If you've something like below -

    public void onApplicationEvent(ContextRefreshedEvent event) {
        // some code
        System.exit(0);
    }

This creates a deadlock as the shutdown hook is created first and it acquires the very lock which System.exit(0) tries acquiring later on. I'm not sure if using System.exit(0) in context refreshed event listener would be discouraged for this reason.

wilkinsona commented 3 years ago

I'm not sure why you'd want to call System.exit(0) in response to a ContextRefreshedEvent. It would be more typical to refresh the application context and then call close() on it. This would then allow the JVM to exit naturally. If, for some reason, you do need to call System.exit(0) when the ContextRefreshEvent is received then you can configure SpringApplication not to register the shutdown hook.

ramtech123 commented 3 years ago

Hi @wilkinsona , I have similar issue as @notjustacoder above, but in my case the exit code is non-zero custom value. Hence context.close() will not work for me. I upgraded from v2.3.4.RELEASE to v2.4.2. I observed my application hanging during exit call and due to that the shell script never received the exit code.

What is the recommended approach to close in such case, as I can't use System.exit(customCode)?

I have come across two possible approaches Runtime.getRuntime().halt(customCode) (which terminates abruptly, may not suite all scenarios) and SpringApplication.exit(ctx, () -> customCode); (yet to test this).

Could you please share your thoughts.

Thanks.

wilkinsona commented 3 years ago

The smallest change would probably be to configure SpringApplication to disable the shutdown hook and continue as you were before. You could also use System.exit(SpringApplication.exit(context)) and use an ExitCodeGenerator to determine the exit code if you prefer. If you have any further questions, please follow up on Stack Overflow or Gitter. As mentioned in the guidelines for contributing, we prefer to use GitHub issues only for bugs and enhancements.

ramtech123 commented 3 years ago

Thanks @wilkinsona , that's helpful.