fix: close vite dev server before creating new one

[tjk]

Description

Without this change, when touching vite config (for example), vite logs [vite] server restarted. and then client reconnect loops because it can never reestablish connection to websocket port (since it was not bound correctly).

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[Shinigami92]

Build is talking about that you need to remove import { prepareError } from './middlewares/error' in L10

[Shinigami92]

question: does this affect #3182 somehow?

[patak-dev]

@tjk please remove the draft status when you are ready to merge

[tjk]

question: does this affect #3182 somehow?

Will test different combinations in a minimal repro and then remove draft status after an update. Thanks!

[tjk]

@Shinigami92 is there a good reason that EADDRINUSE is squelched here: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/ws.ts#L79

If you already have a vite dev server running with default config:

$ yarn workspace test-ssr-vue dev
yarn workspace v1.22.5
yarn run v1.22.5
$ node server
http://localhost:3000
[Vue Router warn]: No match found for location with path "/favicon.ico"
[Vue warn]: <Suspense> slots expect a single root node.
[Vue Router warn]: No match found for location with path "/favicon.ico"
[Vue warn]: <Suspense> slots expect a single root node.
[@vue/compiler-sfc] <script setup> is still an experimental proposal.
Follow its status at https://github.com/vuejs/rfcs/pull/227.

[@vue/compiler-sfc] When using experimental features,
it is recommended to pin your vue dependencies to exact versions to avoid breakage.

[Vue Router warn]: No match found for location with path "/favicon.ico"
[Vue warn]: <Suspense> slots expect a single root node.
[Vue Router warn]: No match found for location with path "/favicon.ico"
[Vue warn]: <Suspense> slots expect a single root node.
[Vue Router warn]: No match found for location with path "/favicon.ico"
[Vue warn]: <Suspense> slots expect a single root node.
[Vue Router warn]: No match found for location with path "/favicon.ico"
[Vue warn]: <Suspense> slots expect a single root node.
[Vue Router warn]: No match found for location with path "/favicon.ico"
[Vue warn]: <Suspense> slots expect a single root node.
[Vue Router warn]: No match found for location with path "/favicon.ico"
[Vue warn]: <Suspense> slots expect a single root node.

So getting the following is helpful:

$ yarn workspace test-ssr-vue dev
yarn workspace v1.22.5
yarn run v1.22.5
$ node server
WebSocket server error:
Error: listen EADDRINUSE: address already in use :::24678
    at Server.setupListenHandle [as _listen2] (net.js:1313:16)
    at listenInCluster (net.js:1361:12)
    at Server.listen (net.js:1447:7)
    at new WebSocketServer (/home/tjk/src/github.com/tjk/vite/packages/vite/dist/node/chunks/dep-e21de3b6.js:61872:20)
    at createWebSocketServer (/home/tjk/src/github.com/tjk/vite/packages/vite/dist/node/chunks/dep-e21de3b6.js:62281:15)
    at Object.createServer (/home/tjk/src/github.com/tjk/vite/packages/vite/dist/node/chunks/dep-e21de3b6.js:73607:16)
    at async createServer (/home/tjk/src/github.com/tjk/vite/packages/playground/ssr-vue/server.js:30:12)
http://localhost:3000
[Vue Router warn]: No match found for location with path "/favicon.ico"
[Vue warn]: <Suspense> slots expect a single root node.
[Vue Router warn]: No match found for location with path "/favicon.ico"
[Vue warn]: <Suspense> slots expect a single root node.
...

[Shinigami92]

@Shinigami92 is there a good reason that EADDRINUSE is squelched here: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/ws.ts#L79

I personally don't know 🤷 You need to ask someone else

[tjk]

@Shinigami92 is there a good reason that EADDRINUSE is squelched here: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/ws.ts#L79

I personally don't know 🤷 You need to ask someone else

Ack. That line is 8 months old. @yyx990803 do you remember a good reason for this?

EDIT: see my comment above... it would be nice for the behavior from the client to also be better but I don't have any obvious ideas yet (in terms of not just spin refreshing if the HMR port is down)

EDIT: (to be clear, this PR currently only addresses re-binding to the same HMR ws port correctly on dev server restart at the moment)

[tjk]

question: does this affect #3182 somehow?

I left comments on that other PR (#3182 (review)).

As it stands, this issue happens in middlewareMode, whereas #3182 only affects root/serve CLI command. This change would affect all restartServer usage obviously... there may be a "better" solution that tries to get at the crux of creating web socket server but easiest fix seems to just ensure it is closed before restarting.

[patak-dev]

I agree, should we mark this one as ready to merge and out of draft then @tjk?

[HomyeeKing]

question: if a Socket disconnect, does the upstream websocket disconnect with it ?

[tjk]

question: if a Socket disconnect, does the upstream websocket disconnect with it ?

I imagine so. Is there a particular scenario you are worried would not work? I'm happy to test cases that might be borked, just let me know please.

[tjk]

I'm away for the day but can check the behavior when I extend the time between close and restart this evening and get back to you.

On Sun, Jul 25, 2021 at 12:13 PM patak ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/vite/src/node/server/hmr.ts <#4374 (comment)>: > @@ -467,18 +466,19 @@ async function readModifiedFile(file: string): Promise<string> { async function restartServer(server: ViteDevServer) { // @ts-ignore global.__vite_start_time = Date.now() + + await server.close() @tjk <https://github.com/tjk> just to check, I think the current pattern was used to avoid missing requests from the client, no? Did you analyze what happened in this case? The user may get a few network errors but the page is reloaded? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4374 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA2VKA5CQ2CETZTR4WBRX3TZRO5LANCNFSM5A4W5A4Q> .

-- TJ Koblentz

[tjk]

@patak-js so during the time between server close and new server is restarted, the client will continuously reconnect at 1 second intervals. This is better than previous behavior and likely shouldn't really trigger a timeout ping... if we want to improve this we could try to signal to client we are restarting so it handles that behavior better or do the socket "handoff" better if possible? Let me know what you think.

(moved to review thread)

[patak-dev]

For reference there is a new issue and PR #4460 related to the way the server is restarting. @tjk did you thought about splitting server.close() into a server.stop() that would return { httpServer, webSocket } and allow these to be used in the next createServer? Not having to recreate this looks like a solution to these issues to me.

I would like to merge this PR and we can continue to explore how to improve here. But I see that we don't have an issue related to it. @tjk would it be possible for you to create a bug report with a minimal reproduction so we can better track this change? Thanks.

[patak-dev]

Tested #4492 and this PR properly fix the issue
Thanks for creating the issue @tjk