-
Notifications
You must be signed in to change notification settings - Fork 112
fix(nuxi): timeout NuxtDevServer.close() to avoid dev-restart deadlock #1328
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
base: main
Are you sure you want to change the base?
Changes from all commits
6086dac
19154ed
acb55cb
a5e0d7c
c0531ff
cff9a05
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 | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -69,6 +69,42 @@ interface NuxtDevServerOptions { | |||||||||||||||||||||||||
| const RESTART_RE = /^(?:nuxt\.config\.[a-z0-9]+|\.nuxtignore|\.nuxtrc|\.config\/nuxt(?:\.config)?\.[a-z0-9]+)$/ | ||||||||||||||||||||||||||
| const TRAILING_SLASH_RE = /\/$/ | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Cap on how long we wait for `nitro.close()` during a dev-server restart. | ||||||||||||||||||||||||||
| // Long-lived plugin connections (Bull `BLPOP`/`BRPOPLPUSH`, Postgres `LISTEN`, WebSocket, | ||||||||||||||||||||||||||
| // shared `ioredis` clients, …) can keep `close()` pending indefinitely; without a cap | ||||||||||||||||||||||||||
| // the dev server stays stuck on "Restarting Nuxt..." forever (see nuxt/nuxt#32928). | ||||||||||||||||||||||||||
| // 3 s is enough for normal close paths (which complete in ms) while still being noticeable | ||||||||||||||||||||||||||
| // and overridable via `NUXT_DEV_CLOSE_TIMEOUT_MS` if a project needs more grace. | ||||||||||||||||||||||||||
| export const DEFAULT_CLOSE_TIMEOUT_MS = 3000 | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * Race `closer()` against a timeout. Resolves either when the closer settles | ||||||||||||||||||||||||||
| * (success or rejection — we don't want a rejected nuxt close to abort the | ||||||||||||||||||||||||||
| * subsequent restart) or when the timer fires, whichever happens first. | ||||||||||||||||||||||||||
| * Exposed for testing; intended to be called from `NuxtDevServer.close()` only. | ||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||
| * The closer is wrapped in `Promise.resolve().then(closer)` so a synchronous | ||||||||||||||||||||||||||
| * throw from the closer is also rerouted through `.catch` and cannot abort | ||||||||||||||||||||||||||
| * the restart. | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
| export async function closeWithTimeout(closer: () => Promise<void>, timeoutMs: number): Promise<void> { | ||||||||||||||||||||||||||
| let timer: NodeJS.Timeout | undefined | ||||||||||||||||||||||||||
| await Promise.race([ | ||||||||||||||||||||||||||
| Promise.resolve() | ||||||||||||||||||||||||||
| .then(closer) | ||||||||||||||||||||||||||
| .catch(() => undefined) | ||||||||||||||||||||||||||
| .finally(() => { | ||||||||||||||||||||||||||
| if (timer) { | ||||||||||||||||||||||||||
| clearTimeout(timer) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||
| new Promise<void>((resolve) => { | ||||||||||||||||||||||||||
| timer = setTimeout(resolve, timeoutMs) | ||||||||||||||||||||||||||
| timer.unref?.() | ||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||
| ]) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export class FileChangeTracker { | ||||||||||||||||||||||||||
| private mtimes = new Map<string, number>() | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -493,9 +529,16 @@ export class NuxtDevServer extends EventEmitter<DevServerEventMap> { | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| async close(): Promise<void> { | ||||||||||||||||||||||||||
| if (this.#currentNuxt) { | ||||||||||||||||||||||||||
| await this.#currentNuxt.close() | ||||||||||||||||||||||||||
| if (!this.#currentNuxt) { | ||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| /* c8 ignore next 4 -- thin delegation to `closeWithTimeout`; that helper is | ||||||||||||||||||||||||||
| unit-tested directly. Reaching this branch from a unit test would require | ||||||||||||||||||||||||||
| mocking the private `#currentNuxt` field which JS-private semantics forbid. */ | ||||||||||||||||||||||||||
| await closeWithTimeout( | ||||||||||||||||||||||||||
| () => this.#currentNuxt!.close(), | ||||||||||||||||||||||||||
| Number(process.env.NUXT_DEV_CLOSE_TIMEOUT_MS) || DEFAULT_CLOSE_TIMEOUT_MS, | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
Comment on lines
+538
to
+541
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. Validate the env timeout before using it.
Suggested fix+ const envTimeout = Number(process.env.NUXT_DEV_CLOSE_TIMEOUT_MS)
+ const timeoutMs = Number.isFinite(envTimeout) && envTimeout > 0
+ ? envTimeout
+ : DEFAULT_CLOSE_TIMEOUT_MS
await closeWithTimeout(
() => this.#currentNuxt!.close(),
- Number(process.env.NUXT_DEV_CLOSE_TIMEOUT_MS) || DEFAULT_CLOSE_TIMEOUT_MS,
+ timeoutMs,
)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** Release the lock file. Call only on final shutdown, not during reloads. */ | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import type { DotenvOptions } from 'c12' | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| import { closeWithTimeout, DEFAULT_CLOSE_TIMEOUT_MS, NuxtDevServer } from '../../src/dev/utils' | ||
|
|
||
| // `closeWithTimeout` is the safety-net behind `NuxtDevServer.close()` — it caps the | ||
| // `nitro.close()` wait so a plugin holding a long-lived connection (Bull `BLPOP`, | ||
| // Postgres `LISTEN`, WebSocket, …) cannot deadlock dev-restart (see nuxt/nuxt#32928). | ||
|
|
||
| describe('closeWithTimeout', () => { | ||
| beforeEach(() => { | ||
| vi.useFakeTimers() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.useRealTimers() | ||
| }) | ||
|
|
||
| it('exposes a non-zero default timeout', () => { | ||
| expect(DEFAULT_CLOSE_TIMEOUT_MS).toBeGreaterThan(0) | ||
| }) | ||
|
|
||
| it('resolves immediately when the closer resolves quickly', async () => { | ||
| const closer = vi.fn().mockResolvedValue(undefined) | ||
| const result = closeWithTimeout(closer, 1000) | ||
| await vi.advanceTimersByTimeAsync(0) | ||
| await expect(result).resolves.toBeUndefined() | ||
| expect(closer).toHaveBeenCalledOnce() | ||
| }) | ||
|
|
||
| it('resolves after the timeout when the closer never settles', async () => { | ||
| // Closer that never resolves — simulates Bull `BLPOP` blocking on Redis. | ||
| const closer = vi.fn(() => new Promise<void>(() => {})) | ||
| const result = closeWithTimeout(closer, 1000) | ||
|
|
||
| // Just before timeout — still pending. | ||
| await vi.advanceTimersByTimeAsync(999) | ||
| // After timeout fires. | ||
| await vi.advanceTimersByTimeAsync(1) | ||
| await expect(result).resolves.toBeUndefined() | ||
| expect(closer).toHaveBeenCalledOnce() | ||
| }) | ||
|
|
||
| it('swallows closer rejections so restart can proceed', async () => { | ||
| const closer = vi.fn().mockRejectedValue(new Error('boom')) | ||
| const result = closeWithTimeout(closer, 1000) | ||
| await vi.advanceTimersByTimeAsync(0) | ||
| await expect(result).resolves.toBeUndefined() | ||
| }) | ||
|
|
||
| it('swallows synchronous throws from closer (so restart can proceed)', async () => { | ||
| const closer = vi.fn(() => { | ||
| throw new Error('sync boom') | ||
| }) as unknown as () => Promise<void> | ||
| const result = closeWithTimeout(closer, 1000) | ||
| await vi.advanceTimersByTimeAsync(0) | ||
| await expect(result).resolves.toBeUndefined() | ||
| }) | ||
|
|
||
| it('does not leave the timer pending after a fast close', async () => { | ||
| const closer = vi.fn().mockResolvedValue(undefined) | ||
| await closeWithTimeout(closer, 60_000) | ||
| // If the timer were still scheduled, advancing the clock would keep the loop alive. | ||
| expect(vi.getTimerCount()).toBe(0) | ||
| }) | ||
| }) | ||
|
|
||
| describe('nuxtDevServer.close', () => { | ||
| it('returns immediately when no Nuxt instance has been initialised yet', async () => { | ||
| // No `init()` call — `#currentNuxt` is unset. The early return guards against | ||
| // crashing if the parent process tears the dev server down before Nuxt loaded. | ||
| const devServer = new NuxtDevServer({ | ||
| cwd: process.cwd(), | ||
| dotenv: {} as DotenvOptions, | ||
| overrides: {}, | ||
| }) | ||
| await expect(devServer.close()).resolves.toBeUndefined() | ||
| }) | ||
| }) |
Uh oh!
There was an error while loading. Please reload this page.