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

[tolgap]

PR Checklist

Please check if your PR fulfills the following requirements:

• 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?

• 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
• No

Other information

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

[coveralls]

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%

Totals
Change from base Build 8bf86286-b462-4455-92cd-c0e58fd6edb9:	0.01%
Covered Lines:	6113
Relevant Lines:	6517

---

💛 - Coveralls

[micalevisk]

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

[tolgap]

@micalevisk it's mentioned under "Other information"

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

[kamilmysliwiec]

Similarly to what Fastify did, should we expose the forceCloseConnections flag to control that behavior?

[tolgap]

I looked into this. It's not really possible with the current ExpressAdapter and FastifyAdapter types.

The way you create a fastify app is by passing new FastifyAdapter(...options). You cannot do this with an express app as ExpressAdapter only takes an express instance.

As this is an option that is passed to the constructor Fastify adapter, we can't replicate this in Express.

Having a forceCloseConnections option on the application makes no sense then, as it would only work for Express.

[kamilmysliwiec]

Do we have any other ideas on how we could avoid introducing a breaking change? AFAIR forceCloseConnections is disabled by default (in Fastify) too.

Perhaps we should expose a dedicated method at the adapter class level? So then you could do

const adapter = app.getHttpAdapter() as ExpressAdapter
adapter.enableForceCloseConnections(); // adapter.forceCloseConnections = true; ?

OR maybe we should auto-enable it when someone calls enableShutdownHooks()?

[tolgap]

@kamilmysliwiec It's just not possible without breaking changes.

For Fastify this option needs to be "available" in the constructor.
For Express, we can enable this behavior afterwards.

Trying to make this option available in the FastifyAdapter constructor would imply a breaking change.

PS: I would love to make a breaking change, just to get this going though 😄 . Let me know what you'd like.

[kamilmysliwiec]

What makes this problematic is the fact that breaking change forces us to release a new major version (and so we'd have to postpone merging this PR a little) :(

[tolgap]

@kamilmysliwiec would you be open to introducing this with difference in API for Fastify? Just like how https is differently configured between express/fastify: https://docs.nestjs.com/faq/multiple-servers

• Express would listen to the forceCloseConnections application option.
• Fastify would ignore this, and only listen to forceCloseConnections if passed into the FastifyAdapter constructor

That way we could document this behavior, and avoid any breaking changes.

[kamilmysliwiec]

SGTM ✅

[tolgap]

@kamilmysliwiec applied the discussed logic.

I added e2e tests for SSE (that extensively use keep-alive connections) for Express + Fastify. Also leveraged the usage of this new option in the 28-sse sample app.

[kamilmysliwiec]

LGTM

[tolgap]

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

[krish]

@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