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

feat(core): Add beforeApplicationShutdown hook #2567

Conversation

BrunnerLivio
Copy link
Member

@BrunnerLivio BrunnerLivio commented Jul 12, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

At the moment only OnApplicationShutdown exists, which gets called once the server stops taking any requests.

What is the new behavior?

BeforeApplicationShutdown gets called once an e.g. system signal got sent to the process, but the app still accepts any requests, as long as all BeforeApplicationShutdown-Promises have been resolved.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

The shutdown order

- OnModuleDestroy: Use to clean up an injectable (unsubscribe Observables, etc.)
- BeforeApplicationShutdown: Gracefully shutdown all the services, but still send unavailable response codes
- OnApplicationShutdown: Shutdown all pending services

Being able to still process requests, even if a system signal has been sent, is crucial in Kubernetes.
@nestjs/terminus can be further improved once this PR gets merged. As soon as beforeApplicationShutdown gets called, Terminus will send unhealthy responses, notifying Kubernetes to switch out the Pod.

I will update the docs once this PR has been merged :)

@@ -81,8 +83,12 @@ export class NestApplicationContext implements INestApplicationContext {
return this;
}

protected async stopServer(): Promise<void> {}

public async close(): Promise<void> {
await this.callDestroyHook();
Copy link
Member Author

Choose a reason for hiding this comment

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

onModuleDestroy hook gets now called before the server stops taking any requests. Before this PR it was called afterward. Is that a problem? I thought the following order makes more sense, that is why I call onModuleDestroy before now.

  1. onModuleDestroy
  2. beforeApplicationShutdown
  3. onApplicationShutdown

@coveralls
Copy link

coveralls commented Jul 12, 2019

Pull Request Test Coverage Report for Build 4157

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.236%

Totals Coverage Status
Change from base Build 4098: 0.0%
Covered Lines: 3638
Relevant Lines: 3820

💛 - Coveralls

@marcus-sa
Copy link

Does the shutdown lifecycle hooks react to SIGTERM automatically, or do we have to call close ourselves to gracefully shutdown the Nest app? This should probably be added to the documentation.

@BrunnerLivio
Copy link
Member Author

BrunnerLivio commented Jul 15, 2019

@marcus-sa the onApplicationShutdown (and beforeApplicationShutdown in the future) will not listen to system signals by default. As far as I remember we decided to make it disabled by default because the application could run into memory leak issues in case multiple Nest instances are running (e.g. testing).

This is why we have added NestApplicationContext.enableShutdownHooks(signals: (ShutdownSignal | string)[] = []), which starts listening on system signals.

public enableShutdownHooks(signals: (ShutdownSignal | string)[] = []): this {
if (isEmpty(signals)) {
signals = Object.keys(ShutdownSignal).map(
(key: string) => ShutdownSignal[key],
);
} else {
// given signals array should be unique because
// process shouldn't listen to the same signal more than once.
signals = Array.from(new Set(signals));
}
signals = signals
.map((signal: string) =>
signal
.toString()
.toUpperCase()
.trim(),
)
// filter out the signals which is already listening to
.filter(signal => !this.activeShutdownSignals.includes(signal));
this.listenToShutdownSignals(signals);
return this;
}

More about this

I can extend the docs further, once this PR gets merged

@@ -220,19 +232,6 @@ export class NestApplication extends NestApplicationContext
});
}

public async close(): Promise<any> {
Copy link
Member

Choose a reason for hiding this comment

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

Why close() is removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It still exists on NestApplicationContext and behaves the same way as before.

NestApplicationContext.close() will just call NestApplicationContext.stopServer(), which in the case of NestApplicationContext will be a noop Promise. Because of inheritance NestApplication will override NestApplicationContext.stopServer and stop the actual server.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh I see await this.stopServer(); in NestApplicationContext now, thanks! I missed that one.

Nonetheless, NestApplicationContext was intended to be platform-agnostic = term "server" will not always exist in case of just "context-based" applications. Maybe we should adjust the naming here?

Copy link
Member Author

Choose a reason for hiding this comment

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

adjust the naming here

Agree. .stopApplication() or just .stop() maybe? I don't particularly like it because stop is just like a synonym for close, but I can't come up with a more descriptive name

Copy link
Member

Choose a reason for hiding this comment

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

Maybe disposeResources() or just dispose()? In the case of HTTP app, resource = server

Copy link
Member Author

Choose a reason for hiding this comment

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

dispose() sounds alright. It is an internal method so we can still change it if we come up with something better :)

@BrunnerLivio BrunnerLivio force-pushed the feature/before-appplication-shutdown-hook branch from fc37541 to b81582d Compare July 19, 2019 10:23
@BrunnerLivio
Copy link
Member Author

@kamilmysliwiec changed stopServer to dispose.

@ninja-
Copy link

ninja- commented Aug 12, 2019

🤔

@BrunnerLivio BrunnerLivio force-pushed the feature/before-appplication-shutdown-hook branch from 0f1124c to 351c780 Compare August 22, 2019 06:54
@BrunnerLivio
Copy link
Member Author

BrunnerLivio commented Aug 22, 2019

Rebased with changes from #2790: Checks isNil instead of isUndefined with beforeApplicationShutdown.


@kamilmysliwiec any reason why this PR has not been merged yet?

@BrunnerLivio BrunnerLivio force-pushed the feature/before-appplication-shutdown-hook branch from 351c780 to 106cdcc Compare August 22, 2019 06:55
@kamilmysliwiec kamilmysliwiec changed the base branch from master to 6.6.0 August 24, 2019 09:13
@kamilmysliwiec kamilmysliwiec modified the milestones: 6.5.4, 6.6.0 Aug 24, 2019
@kamilmysliwiec kamilmysliwiec merged commit a3c47d6 into nestjs:6.6.0 Aug 24, 2019
@lock
Copy link

lock bot commented Nov 22, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants