Skip to content
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: await requests to before server restart #13262

Merged
merged 15 commits into from
Jun 8, 2023

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented May 18, 2023

Description

When a server is restarted, server.moduleGraph will end up being a new clean graph. But server a single object, that we keep across restarts. Outdated plugins and middlewares may be processing requests and if they use server.moduleGraph they could crash or add outdated modules or relationships to the new graph.

This PR awaits processing to be done before doing Object.assign(server, newServer)

We were already awaiting the pluginContainer to close, but this was only calling buildEnd and closeBundle. It nows await for all hooks to finish.

We also await from server._pendingRequests.

See #13231 (comment)

We also discussed with @antfu that a Proxy could be used to give the plugins a view that keeps server.moduleGraph pointing to the old graph. The server proxy that plugins will get in configureServer will be this proxy. I think this could be worth exploring, but if we properly await for the plugins to finish processing, we shouldn't need to do it.

What we may be missing now is to await for external middlewares. We can check this in a future PR.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented May 18, 2023

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

@patak-dev
Copy link
Member Author

patak-dev commented May 18, 2023

Fixed now: I don't know yet why this is failing, I'll review it later. If you spot the issue, let me know :)

@patak-dev patak-dev changed the title fix: await plugins on server restart fix: await requests to before server restart May 19, 2023
@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented May 19, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ❌ failure
nx ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
unocss ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success

@patak-dev patak-dev added the p3-significant High priority enhancement (priority) label May 19, 2023
@patak-dev
Copy link
Member Author

patak-dev commented May 19, 2023

We could revert #13231 after this PR, as old plugins should always see the correct server.moduleGraph but it is probably ok to leave the guard. Maybe we should merge this PR in the beta for 4.4 just in case the changes in server.restart() affect some downstream deps we aren't testing in ecosystem CI.

Note: Nuxt is expected to fail, it is also failing in main now.

@patak-dev patak-dev added this to the 4.4 milestone May 19, 2023
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
patak-dev and others added 2 commits June 6, 2023 17:44
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@@ -46,6 +47,8 @@ export function transformRequest(
server: ViteDevServer,
options: TransformOptions = {},
): Promise<TransformResult | null> {
if (server._restartPromise) throwClosedServerError()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's worth having a method like server.throwIfClosed() to encapsulate this logic and potentially avoid needing to access a private variable. not sure if you'd want to expose this externally, but if so then middlewares could call it if necessary

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting idea. I think we better explore it in a separate PR, and once we have a use case from a middleware to justify exposing the function. transformRequest is internal to vite so I think it is fine reading _restartPromise here for now, and we may find a better way to stop the execution later. Throwing here feels a bit hacky to me to expose it.

@benmccann
Copy link
Collaborator

I left a few small comments, but looks good to me!

benmccann
benmccann previously approved these changes Jun 7, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tiny nits. Looking forward to trying this in the minor!

packages/vite/src/node/server/middlewares/indexHtml.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/middlewares/transform.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/index.ts Show resolved Hide resolved
@bluwy
Copy link
Member

bluwy commented Jun 8, 2023

Maybe we should also rebase the branch before merging just in case we miss a change from main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants