-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Changes from 5 commits
402ace5
716e2fd
e7b5a5a
e802999
6ecee78
592999d
f361ac5
d1b73eb
1bf473f
9f6d6c1
36b43d7
ea13e16
ee8b2d5
bf75322
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@sveltejs/adapter-node": minor | ||
--- | ||
|
||
feat: add shutdown event |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,19 +42,25 @@ if (socket_activation) { | |
}); | ||
} | ||
|
||
function shutdown() { | ||
/** @param {'SIGINT' | 'SIGTERM' | 'IDLE'} reason */ | ||
function shutdown(reason) { | ||
if (shutdown_timeout_id) return; | ||
|
||
// @ts-expect-error this was added in 18.2.0 but is not reflected in the types | ||
server.server.closeIdleConnections(); | ||
benmccann marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
server.server.close(() => { | ||
server.server.close((error) => { | ||
karimfromjordan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (error) return; | ||
benmccann marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (shutdown_timeout_id) { | ||
shutdown_timeout_id = clearTimeout(shutdown_timeout_id); | ||
} | ||
if (idle_timeout_id) { | ||
idle_timeout_id = clearTimeout(idle_timeout_id); | ||
} | ||
|
||
// @ts-expect-error Custom events cannot be typed | ||
benmccann marked this conversation as resolved.
Show resolved
Hide resolved
|
||
process.emit('sveltekit:shutdown', reason); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, got it. I was a bit thrown by the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah makes sense. I just renamed it. |
||
}); | ||
|
||
shutdown_timeout_id = setTimeout( | ||
|
@@ -83,7 +89,7 @@ server.server.on( | |
server.server.closeIdleConnections(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 maybe it'd be better to issue an event here rather than in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was the idea. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I didn't realize that |
||
} | ||
if (requests === 0 && socket_activation && idle_timeout) { | ||
idle_timeout_id = setTimeout(shutdown, idle_timeout * 1000); | ||
idle_timeout_id = setTimeout(() => shutdown('IDLE'), idle_timeout * 1000); | ||
} | ||
}); | ||
} | ||
|
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.
What do you mean by this? When isn't
exit
emitted? I don't see anything about it only happening sometimes in the Node docsmaybe we could say "asynchronous operations" instead of "asynchronous code" to match the wording of the Node docs
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.
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 usingprocess.exit()
the user can't run any async cleanup operations becauseprocess.exit()
forcefully closes the app as quickly as possible.