-
Notifications
You must be signed in to change notification settings - Fork 329
feat(dev): detect 'use cache' module-scope deadlocks early in dev (#1126) #1157
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 5 commits
6103e8c
f0ba0b9
2c7dde8
4b2c506
5662e1a
b4886d1
b543d3a
c2e053f
445a0d0
41895ed
fe5ac07
9dc5254
bc998e6
0b10e4f
3223445
f0a6c58
f15c467
73d4f15
86678d8
242b27e
42fb5cd
fe0d953
9d71da4
fea883d
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 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,7 @@ import { handleApiRoute } from "./server/api-handler.js"; | |||||||||||||||
| import { installSocketErrorBackstop } from "./server/socket-error-backstop.js"; | ||||||||||||||||
| import { shouldInvalidateAppRouteFile } from "./server/dev-route-files.js"; | ||||||||||||||||
| import { createDirectRunner } from "./server/dev-module-runner.js"; | ||||||||||||||||
| import { initUseCacheProbePool, tearDownUseCacheProbePool } from "./server/use-cache-probe-pool.js"; | ||||||||||||||||
| import { generateRscEntry } from "./entries/app-rsc-entry.js"; | ||||||||||||||||
| import { generateSsrEntry } from "./entries/app-ssr-entry.js"; | ||||||||||||||||
| import { generateBrowserEntry } from "./entries/app-browser-entry.js"; | ||||||||||||||||
|
|
@@ -2041,6 +2042,9 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] { | |||||||||||||||
| invalidateAppRouteCache(); | ||||||||||||||||
| invalidateRscEntryModule(); | ||||||||||||||||
| invalidateRootParamsModule(); | ||||||||||||||||
| // Tear down the use-cache probe pool so the next probe starts with | ||||||||||||||||
| // fresh code after HMR invalidation. | ||||||||||||||||
| tearDownUseCacheProbePool(); | ||||||||||||||||
|
Contributor
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. Critical: probe permanently disabled after first HMR. Either re-initialize here:
Suggested change
Or restructure
Contributor
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. The teardown+reinit pattern looks correct now. One thing to watch: if |
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // Node throws on unhandled 'error' events on sockets. When a browser | ||||||||||||||||
|
|
@@ -2134,6 +2138,14 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] { | |||||||||||||||
| // it is flushed to the client. | ||||||||||||||||
| // 2. Logs the full request after res finishes, using those timings. | ||||||||||||||||
| if (hasAppDir) { | ||||||||||||||||
| // Initialize the "use cache" deadlock probe pool for the App | ||||||||||||||||
| // Router dev server. We bind to the RSC environment because | ||||||||||||||||
| // "use cache" functions run inside the RSC module graph. | ||||||||||||||||
| const rscEnv = server.environments["rsc"]; | ||||||||||||||||
| if (rscEnv) { | ||||||||||||||||
| initUseCacheProbePool(rscEnv); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| server.middlewares.use((req, res, next) => { | ||||||||||||||||
| const url = req.url ?? "/"; | ||||||||||||||||
| // Skip Vite internals, HMR, and static assets. | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,7 @@ import type { DevEnvironment } from "vite"; | |
| * environment types — including Cloudflare's custom environments that don't | ||
| * support the hot-channel-based transport. | ||
| */ | ||
| type DevEnvironmentLike = { | ||
| export type DevEnvironmentLike = { | ||
| fetchModule: ( | ||
| id: string, | ||
| importer?: string, | ||
|
|
@@ -129,3 +129,51 @@ export function createDirectRunner(environment: DevEnvironmentLike | DevEnvironm | |
| new ESModulesEvaluator(), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Create a fresh ModuleRunner with its own isolated module graph. | ||
| * | ||
| * Used by the "use cache" deadlock probe to re-import a cache function | ||
| * with zero shared module-scope state from the main request pipeline. | ||
| * Each probe gets a brand-new `EvaluatedModules` instance so top-level | ||
| * `Map`s, `Promise`s, etc. are recreated from scratch. | ||
| * | ||
| * The transport delegates to the same `environment.fetchModule()`, so | ||
| * Vite transforms and HMR still work, but the *evaluated* module cache | ||
| * is completely separate. | ||
| */ | ||
| export function createProbeRunner(environment: DevEnvironmentLike | DevEnvironment): ModuleRunner { | ||
| return new ModuleRunner( | ||
| { | ||
| transport: { | ||
| // oxlint-disable-next-line @typescript-eslint/no-explicit-any | ||
| invoke: async (payload: any) => { | ||
| const { name, data: args } = payload.data; | ||
| if (name === "fetchModule") { | ||
| const [id, importer, options] = args as [ | ||
| string, | ||
| string | undefined, | ||
| { cached?: boolean; startOffset?: number } | undefined, | ||
| ]; | ||
| return { | ||
| result: await environment.fetchModule(id, importer, options), | ||
| }; | ||
| } | ||
| if (name === "getBuiltins") { | ||
| return { result: [] }; | ||
| } | ||
| return { | ||
| error: { | ||
| name: "Error", | ||
| message: `[vinext] Unexpected ModuleRunner invoke: ${name}`, | ||
| }, | ||
| }; | ||
| }, | ||
| }, | ||
| createImportMeta: createNodeImportMeta, | ||
| sourcemapInterceptor: false, | ||
| hmr: false, | ||
| }, | ||
| new ESModulesEvaluator(), | ||
| ); | ||
| } | ||
|
Contributor
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. Moderate: exact duplicate of Just reuse |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,188 @@ | ||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * use-cache-probe-pool.ts | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * Manages isolated ModuleRunners for "use cache" deadlock probes. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * In dev mode, when a cache fill appears stuck, we re-run the same cache | ||||||||||||||||||||||||
| * function in a fresh module graph. If it completes there but the main fill | ||||||||||||||||||||||||
| * is still hung, the hang is attributable to module-scope shared state | ||||||||||||||||||||||||
| * (e.g. a top-level Map used to dedupe fetches) from the outer render. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * Unlike Next.js (which uses jest-worker with real OS processes), vinext | ||||||||||||||||||||||||
| * creates a fresh Vite ModuleRunner per probe. Each runner has its own | ||||||||||||||||||||||||
| * EvaluatedModules instance, so top-level module state is recreated from | ||||||||||||||||||||||||
| * scratch while still using the same Vite transform pipeline. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * The pool is torn down on HMR / file invalidation so the next probe | ||||||||||||||||||||||||
| * starts with fresh transformed code. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import type { DevEnvironment } from "vite"; | ||||||||||||||||||||||||
| import { createProbeRunner, type DevEnvironmentLike } from "./dev-module-runner.js"; | ||||||||||||||||||||||||
| import { ModuleRunner } from "vite/module-runner"; | ||||||||||||||||||||||||
|
Contributor
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.
Suggested change
|
||||||||||||||||||||||||
| import { setUseCacheProbe } from "vinext/shims/use-cache-probe-globals"; | ||||||||||||||||||||||||
| import { UseCacheTimeoutError } from "vinext/shims/use-cache-errors"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let _probeEnvironment: DevEnvironmentLike | DevEnvironment | null = null; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * Initialize the probe pool with the Vite dev environment. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
|
Contributor
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. Nit: JSDoc says "Called once during configureServer()" but the function is also called on every HMR file change (via
Suggested change
|
||||||||||||||||||||||||
| * Called once during configureServer() when the App Router dev server starts. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| export function initUseCacheProbePool(environment: DevEnvironmentLike | DevEnvironment): void { | ||||||||||||||||||||||||
| if (_probeEnvironment) { | ||||||||||||||||||||||||
| // Already initialized — no-op. The environment is the same for the | ||||||||||||||||||||||||
| // lifetime of the dev server. | ||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
|
Comment on lines
+36
to
+39
Contributor
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. Misleading comment. This says "Already initialized — no-op" and "the environment is the same for the lifetime of the dev server", but
Suggested change
|
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| _probeEnvironment = environment; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| setUseCacheProbe(async (msg) => { | ||||||||||||||||||||||||
| // Create a fresh runner per probe so the module graph is completely | ||||||||||||||||||||||||
| // isolated from previous probes. Reusing runners would leave stale | ||||||||||||||||||||||||
| // top-level state in EvaluatedModules. | ||||||||||||||||||||||||
| const runner = createProbeRunner(_probeEnvironment!); | ||||||||||||||||||||||||
| const { id, kind, encodedArguments, request, timeoutMs } = msg; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Internal timeout so the probe aborts before the outer render timeout. | ||||||||||||||||||||||||
| const deadline = Date.now() + timeoutMs; | ||||||||||||||||||||||||
|
Contributor
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. Nit: The
Suggested change
Then line 121 becomes |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| // Import the cache-runtime shim in the isolated runner. | ||||||||||||||||||||||||
| // The shim's registerCachedFunction will create fresh module-scope state. | ||||||||||||||||||||||||
| const cacheRuntime = (await runner.import("vinext/shims/cache-runtime")) as Record< | ||||||||||||||||||||||||
| string, | ||||||||||||||||||||||||
| unknown | ||||||||||||||||||||||||
| >; | ||||||||||||||||||||||||
|
Comment on lines
+60
to
+66
Contributor
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. Minor: The This works correctly because the |
||||||||||||||||||||||||
| const registerCachedFunction = cacheRuntime.registerCachedFunction as | ||||||||||||||||||||||||
| | (<T extends (...args: unknown[]) => Promise<unknown>>( | ||||||||||||||||||||||||
| fn: T, | ||||||||||||||||||||||||
| id: string, | ||||||||||||||||||||||||
| variant?: string, | ||||||||||||||||||||||||
| ) => T) | ||||||||||||||||||||||||
| | undefined; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (!registerCachedFunction) { | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // We need to locate the original cached function module in the isolated | ||||||||||||||||||||||||
| // runner. The function id is "<modulePath>:<exportName>". We split it | ||||||||||||||||||||||||
| // to find the module and the export. | ||||||||||||||||||||||||
| const lastColon = id.lastIndexOf(":"); | ||||||||||||||||||||||||
|
Contributor
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. Edge case: function IDs containing colons in the module path. This is fine for now — just flagging it as a known limitation. A comment noting the assumption ("export names don't contain colons") would be helpful.
Contributor
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. The The comment on line 81 documents the assumption that export names don't contain colons. Worth noting that on Windows, module IDs like
Contributor
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. Windows path edge case. On Windows, Vite module IDs use forward slashes internally, so Not blocking — the assumption is reasonable for file-system modules. But consider adding a brief comment noting this relies on the ID format being |
||||||||||||||||||||||||
| const modulePath = lastColon >= 0 ? id.slice(0, lastColon) : id; | ||||||||||||||||||||||||
| const exportName = lastColon >= 0 ? id.slice(lastColon + 1) : "default"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Import the module containing the original "use cache" function. | ||||||||||||||||||||||||
| const mod = (await runner.import(modulePath)) as Record<string, unknown>; | ||||||||||||||||||||||||
| const originalFn = mod[exportName]; | ||||||||||||||||||||||||
| if (typeof originalFn !== "function") { | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Wrap it with registerCachedFunction so the probe runs through the | ||||||||||||||||||||||||
| // same cache-runtime path (fresh ALS, no shared state). | ||||||||||||||||||||||||
| const variant = kind === "private" ? "private" : ""; | ||||||||||||||||||||||||
|
Contributor
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. Dead code. This should either be removed (simplify to |
||||||||||||||||||||||||
| const wrapped = registerCachedFunction( | ||||||||||||||||||||||||
|
Contributor
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. When If the If that reordering isn't done, consider adding a brief comment here noting the double-timeout is expected and harmless (outer pool timeout preempts inner). |
||||||||||||||||||||||||
| originalFn as (...args: unknown[]) => Promise<unknown>, | ||||||||||||||||||||||||
| id, | ||||||||||||||||||||||||
| variant, | ||||||||||||||||||||||||
|
Contributor
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. Double dev-timeout in probe. Next.js avoids this: when Consider either:
Not blocking since the outer probe pool timeout preempts the inner one, but it's wasteful. |
||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Decode the arguments (simple JSON fallback; RSC encodeReply is | ||||||||||||||||||||||||
| // not available in the probe because we lack the client environment). | ||||||||||||||||||||||||
| // For deadlock detection, the exact argument values matter less than | ||||||||||||||||||||||||
| // the fact that the function body executes with a fresh module scope. | ||||||||||||||||||||||||
| let args: unknown[] = []; | ||||||||||||||||||||||||
| if (typeof encodedArguments === "string") { | ||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| args = JSON.parse(encodedArguments); | ||||||||||||||||||||||||
| if (!Array.isArray(args)) args = [args]; | ||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||
| args = []; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Run the function with a reconstructed request store so private caches | ||||||||||||||||||||||||
| // that read cookies()/headers()/draftMode() see the same values. | ||||||||||||||||||||||||
| // Race against the internal timeout. | ||||||||||||||||||||||||
| const remaining = deadline - Date.now(); | ||||||||||||||||||||||||
|
Contributor
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. If the suggestion to use
Suggested change
Or more simply: const remaining = deadline - performance.now();since the |
||||||||||||||||||||||||
| if (remaining <= 0) { | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| await Promise.race([ | ||||||||||||||||||||||||
| runWithProbeRequestStore(runner, request, async () => wrapped(...args)), | ||||||||||||||||||||||||
| new Promise<never>((_, reject) => { | ||||||||||||||||||||||||
| const t = setTimeout(() => reject(new UseCacheTimeoutError()), remaining); | ||||||||||||||||||||||||
| // Ensure timer is cleaned up on success via unref if available. | ||||||||||||||||||||||||
| if (typeof (t as NodeJS.Timeout).unref === "function") { | ||||||||||||||||||||||||
| (t as NodeJS.Timeout).unref(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||
|
Comment on lines
+110
to
+128
Contributor
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. Redundant await + race: The timeout should wrap the un-awaited call: const result = await Promise.race([
runWithProbeRequestStore(request, async () => wrapped(...args)),
new Promise<never>((_, reject) => {
const t = setTimeout(() => reject(new UseCacheTimeoutError()), remaining);
if (typeof t.unref === 'function') t.unref();
}),
]);
Contributor
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. Minor timer leak inside the probe. When The
Suggested change
Then in the if (probeTimeoutTimer !== undefined) clearTimeout(probeTimeoutTimer); |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||
| // Probe failure is inconclusive — the function might genuinely hang | ||||||||||||||||||||||||
| // even in isolation, or the module import failed. Fall back to the | ||||||||||||||||||||||||
| // regular timeout. | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
|
Contributor
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. A function that throws is not deadlocked. The
Consider distinguishing these:
Suggested change
|
||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||
| runner.close().catch(() => {}); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * Tear down the probe pool. Called on HMR / file invalidation so the next | ||||||||||||||||||||||||
| * probe starts with fresh code. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| export function tearDownUseCacheProbePool(): void { | ||||||||||||||||||||||||
| _probeEnvironment = null; | ||||||||||||||||||||||||
| setUseCacheProbe(undefined); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * Reconstruct a minimal request store in the probe runner so that | ||||||||||||||||||||||||
| * cookies(), headers(), and draftMode() behave correctly. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| async function runWithProbeRequestStore<T>( | ||||||||||||||||||||||||
| runner: ModuleRunner, | ||||||||||||||||||||||||
| requestSnapshot: { | ||||||||||||||||||||||||
| headers: [string, string][]; | ||||||||||||||||||||||||
| cookieHeader: string | undefined; | ||||||||||||||||||||||||
| urlPathname: string; | ||||||||||||||||||||||||
| urlSearch: string; | ||||||||||||||||||||||||
| rootParams: Record<string, unknown>; | ||||||||||||||||||||||||
| isDraftMode: boolean; | ||||||||||||||||||||||||
| isHmrRefresh: boolean; | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| fn: () => Promise<T>, | ||||||||||||||||||||||||
| ): Promise<T> { | ||||||||||||||||||||||||
| // Import the ALS-backed request-context modules through the probe runner | ||||||||||||||||||||||||
| // so they load inside the isolated module graph, not the main runner's. | ||||||||||||||||||||||||
| const { createRequestContext, runWithRequestContext } = (await runner.import( | ||||||||||||||||||||||||
| "vinext/shims/unified-request-context", | ||||||||||||||||||||||||
| )) as typeof import("vinext/shims/unified-request-context"); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const { headersContextFromRequest } = (await runner.import( | ||||||||||||||||||||||||
| "vinext/shims/headers", | ||||||||||||||||||||||||
| )) as typeof import("vinext/shims/headers"); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Build a Request from the snapshot so headersContextFromRequest works. | ||||||||||||||||||||||||
| const url = new URL(requestSnapshot.urlPathname + requestSnapshot.urlSearch, "http://localhost"); | ||||||||||||||||||||||||
| const request = new Request(url, { | ||||||||||||||||||||||||
| headers: new Headers(requestSnapshot.headers), | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const headersContext = headersContextFromRequest(request); | ||||||||||||||||||||||||
| const ctx = createRequestContext({ | ||||||||||||||||||||||||
| headersContext, | ||||||||||||||||||||||||
| executionContext: null, | ||||||||||||||||||||||||
| rootParams: requestSnapshot.rootParams as Record<string, string | string[]>, | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return runWithRequestContext(ctx, fn); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -42,6 +42,8 @@ import { | |||||||||||||||||||||||||||
| getRequestContext, | ||||||||||||||||||||||||||||
| runWithUnifiedStateMutation, | ||||||||||||||||||||||||||||
| } from "./unified-request-context.js"; | ||||||||||||||||||||||||||||
| import { UseCacheTimeoutError, UseCacheDeadlockError } from "./use-cache-errors.js"; | ||||||||||||||||||||||||||||
|
Contributor
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. Production bundle size concern. The PR description says "tree-shakes out of production builds" but that's only true if the bundler can statically determine the Consider either:
|
||||||||||||||||||||||||||||
| import { getUseCacheProbe, type UseCacheProbeRequestSnapshot } from "./use-cache-probe-globals.js"; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // --------------------------------------------------------------------------- | ||||||||||||||||||||||||||||
| // Cache execution context — AsyncLocalStorage for cacheLife/cacheTag | ||||||||||||||||||||||||||||
|
|
@@ -399,7 +401,74 @@ export function registerCachedFunction<T extends (...args: any[]) => Promise<any | |||||||||||||||||||||||||||
| // In dev mode, always execute fresh — skip shared cache lookup/storage. | ||||||||||||||||||||||||||||
| // This ensures HMR changes are reflected immediately. | ||||||||||||||||||||||||||||
| if (isDev) { | ||||||||||||||||||||||||||||
| return executeWithContext(fn, args, cacheVariant); | ||||||||||||||||||||||||||||
| const USE_CACHE_TIMEOUT_MS = 54_000; | ||||||||||||||||||||||||||||
|
Contributor
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. Hardcoded timeout. Next.js reads
Suggested change
|
||||||||||||||||||||||||||||
| const fillDeadlineAt = performance.now() + USE_CACHE_TIMEOUT_MS; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const timeoutError = new UseCacheTimeoutError(); | ||||||||||||||||||||||||||||
|
Contributor
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. Pre-allocating error objects is unusual and slightly wasteful. Consider constructing them lazily — only when the timeout or deadlock actually fires: // In the setTimeout callback:
reject(new UseCacheTimeoutError());
// In the probe .then callback:
reject(new UseCacheDeadlockError());The downside is that the stack trace points to the timer callback instead of the Non-blocking, but worth considering for dev perf if |
||||||||||||||||||||||||||||
| const deadlockError = new UseCacheDeadlockError(); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| let probeTimer: ReturnType<typeof setTimeout> | undefined; | ||||||||||||||||||||||||||||
| let timeoutTimer: ReturnType<typeof setTimeout> | undefined; | ||||||||||||||||||||||||||||
| let probePromise: Promise<never> | null = null; | ||||||||||||||||||||||||||||
| const probe = getUseCacheProbe(); | ||||||||||||||||||||||||||||
|
Contributor
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. Critical: recursive probe via shared The recursion eventually terminates because Fix: guard against recursive probing. Simplest approach — add a flag: let _insideProbe = false;and in the probe callback ( |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (probe) { | ||||||||||||||||||||||||||||
| // Capture the current request store snapshot for the probe. | ||||||||||||||||||||||||||||
| const requestCtx = getRequestContext(); | ||||||||||||||||||||||||||||
| const headers = requestCtx.headersContext?.headers; | ||||||||||||||||||||||||||||
| const navCtx = requestCtx.serverContext; | ||||||||||||||||||||||||||||
| const requestSnapshot: UseCacheProbeRequestSnapshot = { | ||||||||||||||||||||||||||||
| headers: headers ? Array.from(headers.entries()) : [], | ||||||||||||||||||||||||||||
| cookieHeader: headers?.get("cookie") ?? undefined, | ||||||||||||||||||||||||||||
|
Contributor
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.
|
||||||||||||||||||||||||||||
| urlPathname: navCtx?.pathname ?? "/", | ||||||||||||||||||||||||||||
| urlSearch: navCtx?.searchParams?.toString() ?? "", | ||||||||||||||||||||||||||||
| rootParams: requestCtx.rootParams ?? {}, | ||||||||||||||||||||||||||||
| isDraftMode: false, | ||||||||||||||||||||||||||||
| isHmrRefresh: false, | ||||||||||||||||||||||||||||
|
Contributor
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. Minor: |
||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| probePromise = new Promise<never>((_, reject) => { | ||||||||||||||||||||||||||||
|
Contributor
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. The probe timer fires even if execution completes normally, but the reject is harmless. When This is correct — the prior review's timer-leak concern has been addressed. The |
||||||||||||||||||||||||||||
| probeTimer = setTimeout(() => { | ||||||||||||||||||||||||||||
| const probeInternalTimeoutMs = fillDeadlineAt - performance.now() - 1_000; | ||||||||||||||||||||||||||||
| if (probeInternalTimeoutMs <= 0) return; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| probe({ | ||||||||||||||||||||||||||||
| id, | ||||||||||||||||||||||||||||
|
Contributor
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. Non-blocking: |
||||||||||||||||||||||||||||
| kind: cacheVariant, | ||||||||||||||||||||||||||||
|
Contributor
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.
Non-blocking, but removing it would simplify the message and avoid confusion. |
||||||||||||||||||||||||||||
| encodedArguments: JSON.stringify(args), | ||||||||||||||||||||||||||||
|
Contributor
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. Minor:
Suggested change
Alternatively, move the serialization into a helper. |
||||||||||||||||||||||||||||
| request: requestSnapshot, | ||||||||||||||||||||||||||||
| timeoutMs: probeInternalTimeoutMs, | ||||||||||||||||||||||||||||
| }).then( | ||||||||||||||||||||||||||||
| (completed) => { | ||||||||||||||||||||||||||||
| if (completed) reject(deadlockError); | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
| () => {}, | ||||||||||||||||||||||||||||
|
Contributor
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. Inconclusive probe silently dangles. When This isn't a correctness bug (the timeout backstop works), but it defeats the purpose of early detection for the inconclusive case. At minimum, consider logging a dev-mode warning so the developer gets a signal before waiting 44 more seconds:
Suggested change
|
||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| }, 10_000); | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
| // Swallow rejection when execution wins the race. | ||||||||||||||||||||||||||||
| probePromise.catch(() => {}); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||||||||||||||||||||||||||||
|
Contributor
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. Good pattern. The 54s timeout promise is correctly |
||||||||||||||||||||||||||||
| timeoutTimer = setTimeout(() => reject(timeoutError), USE_CACHE_TIMEOUT_MS); | ||||||||||||||||||||||||||||
| if (typeof (timeoutTimer as NodeJS.Timeout).unref === "function") { | ||||||||||||||||||||||||||||
| (timeoutTimer as NodeJS.Timeout).unref(); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
| // Swallow rejection when execution wins the race. | ||||||||||||||||||||||||||||
| timeoutPromise.catch(() => {}); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const executionPromise = executeWithContext(fn, args, cacheVariant); | ||||||||||||||||||||||||||||
|
Contributor
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.
The current order is correct. Just flagging it as a subtlety that a future refactor should preserve.
Contributor
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. Ordering note (for future maintainers): |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const promises: Promise<unknown>[] = [executionPromise]; | ||||||||||||||||||||||||||||
| if (probePromise) promises.push(probePromise); | ||||||||||||||||||||||||||||
| promises.push(timeoutPromise); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return Promise.race(promises).finally(() => { | ||||||||||||||||||||||||||||
|
Contributor
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. The This is masked today by the
Suggested change
const promises: [Promise<Awaited<ReturnType<T>>>, ...Promise<never>[]] = [executionPromise];Non-blocking since the outer signature is |
||||||||||||||||||||||||||||
| if (probeTimer !== undefined) clearTimeout(probeTimer); | ||||||||||||||||||||||||||||
| if (timeoutTimer !== undefined) clearTimeout(timeoutTimer); | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Shared cache ("use cache" / "use cache: remote") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
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.
Minor: static import loads probe pool at plugin registration time.
initUseCacheProbePoolandtearDownUseCacheProbePoolare only used insideconfigureServer()(dev only). A dynamicimport()insideconfigureServerwould avoid loadinguse-cache-probe-pool.js(and its transitive imports ofuse-cache-probe-globalsanduse-cache-errors) duringvite build.Non-blocking — Vite plugins are dev-time artifacts and the module loading cost is trivial. But if bundle size of the plugin itself ever matters, this is low-hanging fruit.