Skip to content

fix: gracefully shutdown preview server on SIGTERM (fix #12990)#17333

Merged
bluwy merged 4 commits intovitejs:mainfrom
elias-pap:fix/gracefully-shutdown-preview-server
Jun 10, 2024
Merged

fix: gracefully shutdown preview server on SIGTERM (fix #12990)#17333
bluwy merged 4 commits intovitejs:mainfrom
elias-pap:fix/gracefully-shutdown-preview-server

Conversation

@elias-pap
Copy link
Copy Markdown
Contributor

@elias-pap elias-pap commented May 28, 2024

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 code 1, and on the PR branch with code 0.

Main branch manual test:

main.mov

PR branch manual test:

fix.mov

@bolt-new-by-stackblitz
Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@elias-pap
Copy link
Copy Markdown
Contributor Author

elias-pap commented May 28, 2024

If tests are needed for this fix, please let me know about the preferred place to put them.

@elias-pap elias-pap changed the title fix: gracefully shutdown preview server on SIGTERM (fix #12990) fix: gracefully shutdown preview server on SIGTERM (fix #12990) May 28, 2024
Comment on lines +1441 to +1455

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)
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

@elias-pap elias-pap Jun 5, 2024

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.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

@elias-pap elias-pap Jun 6, 2024

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.

Copy link
Copy Markdown
Contributor Author

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.

@patak-cat
Copy link
Copy Markdown
Member

Thanks for the PR! This looks good to me, let's avoid creating the abstractions I mention and we can check what others think.

@elias-pap elias-pap requested a review from patak-cat June 6, 2024 16:37
patak-cat
patak-cat previously approved these changes Jun 6, 2024
@bluwy bluwy merged commit 2207a68 into vitejs:main Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gracefully shutdown preview server when receiving SIGTERM

3 participants