OnApplicationShutdown hook

[JulienMichelFr]

I'm submitting a...

[ ] Regression
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

I have an app with the Terminus module and the app also have the shutdown hooks enabled.
When I try to stop the app with a SIGTERM, the underlying http server is closed before the shutdown hooks are executed. It also trigger two times the shutdown hooks.

[Nest] 31043   - 08/07/2019, 2:37 PM   [NestApplication] Nest application successfully started +4ms
[Nest] 31043   - 08/07/2019, 2:37 PM   [OnApplicationShutdown] Started at Wed Aug 07 2019 14:37:41 GMT+0200 (Central European Summer Time) +6318ms
// The http server is stop somewhere here.
[Nest] 31043   - 08/07/2019, 2:37 PM   [OnApplicationShutdown] Started at Wed Aug 07 2019 14:37:42 GMT+0200 (Central European Summer Time) +1007ms
[Nest] 31043   - 08/07/2019, 2:37 PM   [OnApplicationShutdown] Completed at Wed Aug 07 2019 14:37:48 GMT+0200 (Central European Summer Time) +6352ms
[Nest] 31043   - 08/07/2019, 2:37 PM   [OnApplicationShutdown] Completed at Wed Aug 07 2019 14:37:48 GMT+0200 (Central European Summer Time) +1ms

Expected behavior

The http server should stay until all shutdown hooks are resolved and the onApplicationShutdown hook should be call only one time.

Minimal reproduction of the problem with instructions

Clone this repo

Compile the app with npm run build then start it.

Curl the local server with curl --location --request GET "localhost:3000/" and stop the process with a SIGTERM while request is being processed.

What is the motivation / use case for changing the behavior?

The server should be able to process already accepted requests before shutting down.

Environment

Nest version: 6.5.3
@nestjs/terminus version: 6.5.0

 
For Tooling issues:
- Node version: 10.16.0 
- Platform:  MacOS, Containerized ubuntu 18

Possible solution :

The problem seems to come from there with this not allowing the configuration of the beforeShutdown hook on godaddy/terminus. Allowing user to configure the beforeShutdown hook here or setting a promise based timeout should fix the http server problem.

[BrunnerLivio]

Thank you for your issue. Nice digging!
I was already aware of this problem. Unfortunately I forgot to submit an issue - sorry 'bout that.
Anyhow. I am currently blocked due to nestjs/nest#2567 which is required to solve this problem in an elegant way.

Allowing user to configure the beforeShutdown hook here or setting a promise based timeout should fix the http server problem.

I do not think this would solve the problem in a clean way. My strategy will be once beforeApplicationShutdown is merged, to fully discard the system signal handling by @godaddy/terminus. It makes more sense to handle it ourselves.

How urgent is this issue for you - since I can't control when my PR gets merged. Otherwise I could make it workaround-able for the time being.

[JulienMichelFr]

Thank you for your quick response.
Since I found the issue while prototyping, I can wait for your PR to be merged.

[galkin]

@BrunnerLivio, I opened pull with the fix for this issue. Could you make a review?

[BrunnerLivio]

Resolved in 7.0.0 🎉