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

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

Closed
StoffelCPR opened this issue Oct 9, 2020 · 11 comments
Labels
type: bug A general bug
Milestone

Comments

@StoffelCPR
Copy link

StoffelCPR commented Oct 9, 2020

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:
liquibase/liquibase#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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 9, 2020
@wilkinsona
Copy link
Member

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.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue for: team-attention An issue we'd like other members of the team to review labels Oct 9, 2020
@StoffelCPR
Copy link
Author

StoffelCPR commented Oct 13, 2020

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.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 13, 2020
@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 13, 2020
@StoffelCPR
Copy link
Author

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.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 14, 2020
@philwebb
Copy link
Member

See #4053 for some more background

@philwebb philwebb added type: bug A general bug and removed for: team-attention An issue we'd like other members of the team to review status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Oct 14, 2020
@philwebb philwebb modified the milestones: 2.3.x, 2.2.x Oct 14, 2020
@wilkinsona wilkinsona changed the title ShutdownHook is registered after liquibase is done. If the JVM is killed while refresh is in progress, the shut down hook does not close the context Oct 14, 2020
@StoffelCPR
Copy link
Author

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
Copy link
Member

bclozel commented Oct 14, 2020

@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.

@wilkinsona wilkinsona changed the title If the JVM is killed while refresh is in progress, the shut down hook does not close the context If the JVM is killed while refresh is in progress, the shutdown hook does not close the context Oct 15, 2020
@wilkinsona wilkinsona modified the milestones: 2.2.x, 2.2.11 Oct 19, 2020
@notjustacoder
Copy link

notjustacoder commented Mar 4, 2021

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
Copy link
Member

wilkinsona commented Mar 4, 2021

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
Copy link

ramtech123 commented Mar 5, 2021

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
Copy link
Member

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
Copy link

Thanks @wilkinsona , that's helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

7 participants