Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: adapter-node shutdown event #12153
feat: adapter-node shutdown event #12153
Changes from all commits
402ace5
716e2fd
e7b5a5a
e802999
6ecee78
592999d
f361ac5
d1b73eb
1bf473f
9f6d6c1
36b43d7
ea13e16
ee8b2d5
bf75322
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that this doesn't work with the graceful shutdown. If the user listens for the
SIGINT
andSIGTERM
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, got it. I was a bit thrown by the
process.on('SIGTERM', shutdown);
line. Do you think we could renameshutdown
tograceful_shutdown
to make it a bit clearer in the code that it's not shutting down immediately?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah makes sense. I just renamed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm. I wonder if users would want an event to know that all requests have finished processing. if you issue a
sveltekit:shutdown
when receivingSIGINT
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 forSIGTERM
orSIGINT
individually without us firing an event that simply echoes them (/~https://github.com/sveltejs/kit/pull/12153/files#r1637507349)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the idea. The
sveltekit:shutdown
event is emitted inside theserver.close((error) => { ... })
callback. And that callback runs after all requests/connections have finished and the server closed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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