nest
nest copied to clipboard
fix(express): shutdown hooks not firing caused by open http connections
PR Checklist
Please check if your PR fulfills the following requirements:
- [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
PR Type
What kind of change does this PR introduce?
- [x] Bugfix
- [ ] 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?
Issue Number: #9517
When using long living HTTP connections (like SSE/Keep-Alive connecitons), in conjuction with app.enableShutdownHooks() exposed a flaw. When there are keep alive connections in our HTTP server, they would never get closed. This would cause our HTTP adapter to endlessly wait on them until a second restart.
Because the shutdown hooks would halt at httpAdapter.close(), the rest of the application shutdown logic would also never run.
What is the new behavior?
We keep track of open connections in our Express adapter. When httpAdapter.close() is called, we iterate these open connections and close them.
Does this PR introduce a breaking change?
- [ ] Yes
- [x] No
Other information
On that note: this fix is superfluous for Fastify. Fastify has the forceCloseConnections
Pull Request Test Coverage Report for Build b1ac06a7-06be-4303-8dfa-9dccc7e3e568
- 0 of 0 changed or added relevant lines in 0 files are covered.
- 47 unchanged lines in 7 files lost coverage.
- Overall coverage increased (+0.01%) to 93.801%
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| packages/core/router/router-execution-context.ts | 1 | 97.69% |
| packages/core/router/routes-resolver.ts | 4 | 93.02% |
| packages/microservices/server/server-tcp.ts | 4 | 76.74% |
| packages/microservices/server/server-mqtt.ts | 7 | 80.74% |
| packages/core/injector/module.ts | 9 | 85.34% |
| packages/core/injector/container.ts | 10 | 89.05% |
| packages/core/router/router-explorer.ts | 12 | 78.95% |
| <!-- | Total: | 47 |
| Totals | |
|---|---|
| Change from base Build 8bf86286-b462-4455-92cd-c0e58fd6edb9: | 0.01% |
| Covered Lines: | 6113 |
| Relevant Lines: | 6517 |
💛 - Coveralls
do you know if this is an issue with express adapter only?
@micalevisk it's mentioned under "Other information"
Also on that note: this fix is superfluous for Fastify. Fastify has the forceCloseConnections
LGTM
@kamilmysliwiec I will start working on the documentation page for this.
@tolgap @kamilmysliwiec what is the effective version for this fix? is there anyway to patch Nest 8 with this fix? because graceful shutdown does not work as expected even call app.close() it stucked when it has http connections