Skip to content

Provide a constant for graceful shutdown's smart lifecycle phase #24255

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

Closed
joca-bt opened this issue Nov 25, 2020 · 5 comments
Closed

Provide a constant for graceful shutdown's smart lifecycle phase #24255

joca-bt opened this issue Nov 25, 2020 · 5 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@joca-bt
Copy link
Contributor

joca-bt commented Nov 25, 2020

Upon graceful shutdown the application will wait for a configurable timeout before killing the webserver and continuing with the termination of the rest of the application. Graceful shutdown is currently happening after ContextClosedEvent is published and before beans are destroyed, during lifecycle stop.

Our application publishes information about all serviced requests to an external data source. We're typically not interested in the requests that were terminated inflight by the graceful shutdown but are interested in the ones that finished after the shutdown was triggered. The logger publishing this information is async and we would like to flush it after the webserver has finished servicing all live requests and has been terminated and only then terminate the logger.

Currently we can achieve this with a SmartLifecycle bean whose phase is later than that of bean webServerGracefulShutdown. Bean webServerGracefulShutdown has the default phase but this is not documented and the class is not public, so this feels a bit hacky. Alternatively, we could terminate our logger on ContextClosedEvent. However, this way we would be dropping the logging of all the requests that finished being serviced during graceful shutdown.

There's a WebServerInitializedEvent. Would having a similar event for termination make sense?

I think @PreDestroy would also be an option but I'm not sure it might be the desired interface in this or similar situations.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 25, 2020
@snicoll

This comment has been minimized.

@philwebb philwebb added for: team-meeting An issue we'd like to discuss as a team to make progress type: enhancement A general enhancement and removed for: team-meeting An issue we'd like to discuss as a team to make progress status: waiting-for-triage An issue we've not yet triaged labels Dec 1, 2020
@philwebb philwebb added this to the 2.5.x milestone Dec 7, 2020
@philwebb philwebb added the status: pending-design-work Needs design work before any code can be developed label Dec 7, 2020
@philwebb
Copy link
Member

philwebb commented Dec 7, 2020

We're not sure about adding a new event, but we do want to improve the documentation and perhaps add some constants so that things are clearer.

@philwebb philwebb removed the status: pending-design-work Needs design work before any code can be developed label Mar 8, 2021
@philwebb
Copy link
Member

philwebb commented Mar 8, 2021

Putting the constants on WebServerGracefulShutdownLifecycle would be an option if we make the class public but keep the constructor package-private. We'd also make the class final.

@wilkinsona wilkinsona changed the title Extending graceful shutdown callbacks Provide a constant for graceful shutdown's smart lifecycle phase Mar 19, 2021
@wilkinsona
Copy link
Member

We've got two smart lifecycle classes at the moment:

  • org.springframework.boot.web.reactive.context.WebServerGracefulShutdownLifecycle
  • org.springframework.boot.web.servlet.context.WebServerGracefulShutdownLifecycle

It might be a bit confusing to have a constant defined on both of them. The alternative would be a common smart lifecycle in org.springframework.boot.context that can be used with the reactive and servlet-based code. This would, however, require the constructor to be public. Here's what that looks like.

We were keen to keep the constructors package-private. Flagging for team attention to see if we'd rather keep the separate smart lifecycles with package-private constructors and have two constants, or a single smart lifecycle and constant but with a public constructor.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Mar 19, 2021
@philwebb
Copy link
Member

philwebb commented Mar 22, 2021

I like the single class, it seems less confusing than duplicated constants.

@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Mar 23, 2021
@wilkinsona wilkinsona self-assigned this Mar 23, 2021
@snicoll snicoll modified the milestones: 2.5.x, 2.5.0-RC1 Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants