feat: adapter-node shutdown event

[karimfromjordan]

This is a small improvement and the missing piece to the recent graceful shutdown feature.
After the server has closed gracefully Node will emit the exit event. As others have pointed out however if you have something running in the background the app will continue to run and exit will never be emitted. The exit event isn't ideal anyway because it will close your app as soon as possible so we can't do any custom async cleanup work in a process.on('exit', () => { ... }) handler. That means calling process.exit() to manually emit the exit event isn't an option. It defeats the point of a graceful shutdown. This PR emits a custom event instead that users of adapter-node can listen to and use to perform any kind of cleanup work right after the HTTP server has closed.

process.on('sveltekit:shutdown', async (reason) => {
  await jobs.stop();
  await db.close();
});

We briefly discussed the option of exporting a function to register a hook instead but with the way adapters work this doesn't seem to be possible today.

---

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: /~https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

• Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

[changeset-bot]

🦋 Changeset detected

Latest commit: bf75322

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/adapter-node	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[benmccann]

Unlike Node's exit event, the sveltekit:shutdown event is always emitted when a shutdown is triggered

What do you mean by this? When isn't exit emitted? I don't see anything about it only happening sometimes in the Node docs

and also supports handling asynchronous code.

maybe we could say "asynchronous operations" instead of "asynchronous code" to match the wording of the Node docs

[karimfromjordan]

exit is emitted automatically when Node has no work left to do. But there might be the scenario where the SvelteKit server shuts down because we listen for a SIGINT but the Node app still doesn't close because the user has a Mongo DB connection in the background that prevents it. That's what's happening in the comment I linked to at the beginning. In that case, even though the SvelteKit server closed, an exit event is not emitted. And if we manually emit the exit event after the server has closed using process.exit() the user can't run any async cleanup operations because process.exit() forcefully closes the app as quickly as possible.

[benmccann]

I'm a bit torn as to whether this is the right API. It is more convenient to work with but kind of duplicating/aggregating existing events. The alternative would be to have the user do this:

process.on('SIGINT', shutdownHandler)
process.on('SIGTERM', shutdownHandler)
process.on('sveltekit:idle', shutdownHandler)

I think what you're doing is probably nicer, but I thought I'd note it for posterity or in case other reviewers have a different opinion

[karimfromjordan]

The problem is that this doesn't work with the graceful shutdown. If the user listens for the SIGINTand SIGTERM events to run some cleanup code then that cleanup code will run at the same time as the server is shutting down and when some requests might not have finished yet. But ideally you want to close the db connection etc. after the server has closed.

[benmccann]

ah, got it. I was a bit thrown by the process.on('SIGTERM', shutdown); line. Do you think we could rename shutdown to graceful_shutdown to make it a bit clearer in the code that it's not shutting down immediately?

[karimfromjordan]

Yeah makes sense. I just renamed it.

[benmccann]

hmm. I wonder if users would want an event to know that all requests have finished processing. if you issue a sveltekit:shutdown when receiving SIGINT and shutdown your database connections then maybe your requests can't actually finish and fail because the database connection is gone, which would kind of defeat the purpose of having a shutdown timeout.

maybe it'd be better to issue an event here rather than in shutdown. if users want to do something earlier they can still listen for SIGTERM or SIGINT individually without us firing an event that simply echoes them (/~https://github.com/sveltejs/kit/pull/12153/files#r1637507349)

[karimfromjordan]

That was the idea. The sveltekit:shutdown event is emitted inside the server.close((error) => { ... }) callback. And that callback runs after all requests/connections have finished and the server closed.

[benmccann]

Ah, I didn't realize that server.close() was graceful. I added suggestion for a comment here: /~https://github.com/sveltejs/kit/pull/12153/files#r1638706765

[benmccann]

Same as above basically... I don't understand exactly what this is doing. If we call .close() isn't that closing idle connections already? Why handle it here as well?

[karimfromjordan]

If a connection was opened with a keep-alive header close() will wait for the connection to time out rather than close it early even if the connection is not handling any requests at the moment. That's one of the reasons why closeIdleConnections() was added to Node.js which also closes idle keep-alive connections. So to make sure that all idle connections are closed as soon as possible we should call both methods, see also nodejs/node#41578

[karimfromjordan]

Btw. I added that commit today because it occurred to me that if we don't close the connections asap it could theoretically happen that a connection stays open, continues to receive requests and therefor later has to be forcefully closed.

[benmccann]

awesome. great job covering the bases on this! that'd be so easy to miss. it's really great you figured out that needed to be done

[benmccann]

really great job on this!

thank you so much for seeing this through and being patient with me trying to handle our long queue of PRs to review. I also appreciate the updates to help with readability and comments since we don't have tests for this package yet! one of many things I should probably get to one day...