nest icon indicating copy to clipboard operation
nest copied to clipboard

fix(express): shutdown hooks not firing caused by open http connections

Open tolgap opened this issue 3 years ago • 3 comments
trafficstars

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

tolgap avatar Oct 03 '22 17:10 tolgap

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 Coverage Status
Change from base Build 8bf86286-b462-4455-92cd-c0e58fd6edb9: 0.01%
Covered Lines: 6113
Relevant Lines: 6517

💛 - Coveralls

coveralls avatar Oct 03 '22 17:10 coveralls

do you know if this is an issue with express adapter only?

micalevisk avatar Oct 03 '22 18:10 micalevisk

@micalevisk it's mentioned under "Other information"

Also on that note: this fix is superfluous for Fastify. Fastify has the forceCloseConnections

tolgap avatar Oct 03 '22 18:10 tolgap

LGTM

kamilmysliwiec avatar Nov 07 '22 10:11 kamilmysliwiec

@kamilmysliwiec I will start working on the documentation page for this.

tolgap avatar Nov 07 '22 10:11 tolgap

@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

krish avatar May 07 '23 05:05 krish