-
-
Notifications
You must be signed in to change notification settings - Fork 6.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
fix: gracefully shutdown preview server on SIGTERM
(fix #12990)
#17333
fix: gracefully shutdown preview server on SIGTERM
(fix #12990)
#17333
Conversation
Run & review this pull request in StackBlitz Codeflow. |
If tests are needed for this fix, please let me know about the preferred place to put them. |
SIGTERM
(fix #12990)
packages/vite/src/node/utils.ts
Outdated
|
||
export const closeServerAndExit = async ( | ||
server: ViteDevServer | PreviewServer | PreviewServer['httpServer'], | ||
): Promise<void> => { | ||
try { | ||
await server.close() | ||
} finally { | ||
process.exit() | ||
} | ||
} | ||
|
||
export const createCloseServerAndExitFn = | ||
(server: ViteDevServer | PreviewServer): (() => Promise<void>) => | ||
() => | ||
closeServerAndExit(server) |
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 think we can avoid these two abstractions and directly inline the code. It is just a little bit more verbose but we don't need to check what is going on inside.
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 agree about createCloseServerAndExitFn()
, we can inline that and avoid the mental load of the closure, but could we just keep closeServerAndExit()
? It is used in 4 places (2 times in shortcuts + dev and preview server), I find it slightly better to avoid having the same piece of code 4 times.
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 the overloading a bit complex. The explicit try finally block very clear to me, and the repetition doesn't bothers the reader.
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.
Addressed in 9e32ee2. closeServerAndExit()
in index and preview server is needed so the same reference can be passed in setup and teardown functions.
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.
This run looks like a flake, it's passing locally.
Thanks for the PR! This looks good to me, let's avoid creating the abstractions I mention and we can check what others think. |
Description
fixes #12990
In the following manual tests the
alias
playground is opened in the left terminal, which runs the preview server.In the right terminal, a
SIGTERM
signal is sent (kill -15
) to the left one.On the main branch, the
preview
command exits with code1
, and on the PR branch with code0
.Main branch manual test:
main.mov
PR branch manual test:
fix.mov