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
16 changes: 10 additions & 6 deletions packages/vite/src/node/plugins/importAnalysis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
shouldExternalizeForSSR,
} from '../ssr/ssrExternal'
import { getDepsOptimizer, optimizedDepNeedsInterop } from '../optimizer'
import { ERR_CLOSED_SERVER } from '../server/pluginContainer'
import { checkPublicFile } from './asset'
import {
ERR_OUTDATED_OPTIMIZED_DEP,
Expand Down Expand Up @@ -255,10 +256,10 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
// have been loaded so its entry is guaranteed in the module graph.
const importerModule = moduleGraph.getModuleById(importer)!
if (!importerModule) {
// When the server is restarted, the module graph is cleared, so we
// return without transforming. This request is no longer valid, a full reload
// is going to request this id again. Throwing an outdated error so we
// properly finish the request with a 504 sent to the browser.
// This request is no longer valid. It could happen for optimized deps
// requests. A full reload is going to request this id again.
// Throwing an outdated error so we properly finish the request with a
// 504 sent to the browser.
throwOutdatedRequest(importer)
}

Expand Down Expand Up @@ -650,8 +651,11 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
// by the deps optimizer
const url = removeImportQuery(hmrUrl)
server.transformRequest(url, { ssr }).catch((e) => {
if (e?.code === ERR_OUTDATED_OPTIMIZED_DEP) {
// This are expected errors
if (
e?.code === ERR_OUTDATED_OPTIMIZED_DEP ||
e?.code === ERR_CLOSED_SERVER
) {
// these are expected errors
return
}
// Unexpected error, log the issue but avoid an unhandled exception
Expand Down
11 changes: 11 additions & 0 deletions packages/vite/src/node/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,17 @@ export async function _createServer(
getDepsOptimizer(server.config, true)?.close(),
closeHttpServer(),
])
// Await pending requests. We throw early in transformRequest
// and in hooks if the server is closing, so the import analysis
// plugin stops pre-transforming static imports and this block
// is resolved sooner.
while (server._pendingRequests.size > 0) {
await Promise.allSettled(
[...server._pendingRequests.values()].map(
(pending) => pending.request,
),
)
}
patak-dev marked this conversation as resolved.
Show resolved Hide resolved
bluwy marked this conversation as resolved.
Show resolved Hide resolved
server.resolvedUrls = null
},
printUrls() {
Expand Down
9 changes: 9 additions & 0 deletions packages/vite/src/node/server/middlewares/indexHtml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import {
unwrapId,
wrapId,
} from '../../utils'
import { ERR_CLOSED_SERVER } from '../pluginContainer'
import { ERR_OUTDATED_OPTIMIZED_DEP } from '../../plugins/optimizedDeps'
import { isCSSRequest } from '../../plugins/css'
import { checkPublicFile } from '../../plugins/asset'
import { getCodeWithSourcemap, injectSourcesContent } from '../sourcemap'
Expand Down Expand Up @@ -349,6 +351,13 @@ function preTransformRequest(server: ViteDevServer, url: string, base: string) {

// transform all url as non-ssr as html includes client-side assets only
server.transformRequest(url).catch((e) => {
if (
e?.code === ERR_OUTDATED_OPTIMIZED_DEP ||
e?.code === ERR_CLOSED_SERVER
) {
// these are expected errors
return
}
// Unexpected error, log the issue but avoid an unhandled exception
server.config.logger.error(e.message)
})
Expand Down
16 changes: 16 additions & 0 deletions packages/vite/src/node/server/middlewares/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
ERR_OPTIMIZE_DEPS_PROCESSING_ERROR,
ERR_OUTDATED_OPTIMIZED_DEP,
} from '../../plugins/optimizedDeps'
import { ERR_CLOSED_SERVER } from '../pluginContainer'
import { getDepsOptimizer } from '../../optimizer'

const debugCache = createDebugger('vite:cache')
Expand Down Expand Up @@ -234,6 +235,21 @@ export function transformMiddleware(
// error but a normal part of the missing deps discovery flow
return
}
if (e?.code === ERR_CLOSED_SERVER) {
// Skip if response has already been sent
if (!res.writableEnded) {
res.statusCode = 504 // status code request timeout
res.statusMessage = 'Outdated Request'
res.end()
}
// We don't need to log an error in this case, the request
// is outdated because new dependencies were discovered and
// the new pre-bundle dependencies have changed.
// A full-page reload has been issued, and these old requests
// can't be properly fulfilled. This isn't an unexpected
// error but a normal part of the missing deps discovery flow
return
}
if (e?.code === ERR_LOAD_URL) {
// Let other middleware handle if we can't load the url via transformRequest
return next()
Expand Down
70 changes: 54 additions & 16 deletions packages/vite/src/node/server/pluginContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,18 @@ import { createPluginHookUtils } from '../plugins'
import { buildErrorMessage } from './middlewares/error'
import type { ModuleGraph } from './moduleGraph'

export const ERR_CLOSED_SERVER = 'ERR_CLOSED_SERVER'

export function throwClosedServerError(): never {
const err: any = new Error(
'The server is being restarted or closed. Request is outdated',
)
err.code = ERR_CLOSED_SERVER
// This error will be caught by the transform middleware that will
// send a 504 status code request timeout
throw err
}

export interface PluginContainerOptions {
cwd?: string
output?: OutputOptions
Expand Down Expand Up @@ -195,6 +207,7 @@ export async function createPluginContainer(
): Promise<void> {
const parallelPromises: Promise<unknown>[] = []
for (const plugin of getSortedPlugins(hookName)) {
// Don't throw here if closed, so buildEnd and closeBundle hooks can finish running
const hook = plugin[hookName]
if (!hook) continue
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
Expand Down Expand Up @@ -571,12 +584,26 @@ export async function createPluginContainer(
}

let closed = false
const processesing = new Set<Promise<any>>()
// keeps track of hook promises so that we can wait for them all to finish upon closing the server
function handleHookPromise<T>(maybePromise: undefined | T | Promise<T>) {
patak-dev marked this conversation as resolved.
Show resolved Hide resolved
if (!(maybePromise as any)?.then) {
return maybePromise
}
const promise = maybePromise as Promise<T>
processesing.add(promise)
return promise.finally(() => processesing.delete(promise))
}

const container: PluginContainer = {
options: await (async () => {
let options = rollupOptions
for (const optionsHook of getSortedPluginHooks('options')) {
options = (await optionsHook.call(minimalContext, options)) || options
if (closed) throwClosedServerError()
options =
(await handleHookPromise(
optionsHook.call(minimalContext, options),
)) || options
}
if (options.acornInjectPlugins) {
parser = acorn.Parser.extend(
Expand All @@ -593,10 +620,12 @@ export async function createPluginContainer(
getModuleInfo,

async buildStart() {
await hookParallel(
'buildStart',
(plugin) => new Context(plugin),
() => [container.options as NormalizedInputOptions],
await handleHookPromise(
hookParallel(
'buildStart',
(plugin) => new Context(plugin),
() => [container.options as NormalizedInputOptions],
),
)
},

Expand All @@ -609,10 +638,10 @@ export async function createPluginContainer(
ctx._scan = scan
ctx._resolveSkips = skip
const resolveStart = debugResolve ? performance.now() : 0

let id: string | null = null
const partial: Partial<PartialResolvedId> = {}
for (const plugin of getSortedPlugins('resolveId')) {
if (closed) throwClosedServerError()
if (!plugin.resolveId) continue
if (skip?.has(plugin)) continue

Expand All @@ -623,13 +652,15 @@ export async function createPluginContainer(
'handler' in plugin.resolveId
? plugin.resolveId.handler
: plugin.resolveId
const result = await handler.call(ctx as any, rawId, importer, {
assertions: options?.assertions ?? {},
custom: options?.custom,
isEntry: !!options?.isEntry,
ssr,
scan,
})
const result = await handleHookPromise(
handler.call(ctx as any, rawId, importer, {
assertions: options?.assertions ?? {},
custom: options?.custom,
isEntry: !!options?.isEntry,
ssr,
scan,
}),
)
if (!result) continue

if (typeof result === 'string') {
Expand Down Expand Up @@ -675,11 +706,14 @@ export async function createPluginContainer(
const ctx = new Context()
ctx.ssr = !!ssr
for (const plugin of getSortedPlugins('load')) {
if (closed) throwClosedServerError()
if (!plugin.load) continue
ctx._activePlugin = plugin
const handler =
'handler' in plugin.load ? plugin.load.handler : plugin.load
const result = await handler.call(ctx as any, id, { ssr })
const result = await handleHookPromise(
handler.call(ctx as any, id, { ssr }),
)
if (result != null) {
if (isObject(result)) {
updateModuleInfo(id, result)
Expand All @@ -696,6 +730,7 @@ export async function createPluginContainer(
const ctx = new TransformContext(id, code, inMap as SourceMap)
ctx.ssr = !!ssr
for (const plugin of getSortedPlugins('transform')) {
if (closed) throwClosedServerError()
if (!plugin.transform) continue
ctx._activePlugin = plugin
ctx._activeId = id
Expand All @@ -707,7 +742,9 @@ export async function createPluginContainer(
? plugin.transform.handler
: plugin.transform
try {
result = await handler.call(ctx as any, code, id, { ssr })
result = await handleHookPromise(
handler.call(ctx as any, code, id, { ssr }),
)
} catch (e) {
ctx.error(e)
}
Expand Down Expand Up @@ -741,6 +778,8 @@ export async function createPluginContainer(

async close() {
if (closed) return
closed = true
await Promise.allSettled(Array.from(processesing))
const ctx = new Context()
await hookParallel(
'buildEnd',
Expand All @@ -752,7 +791,6 @@ export async function createPluginContainer(
() => ctx,
() => [],
)
closed = true
},
}

Expand Down
11 changes: 9 additions & 2 deletions packages/vite/src/node/server/transformRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { checkPublicFile } from '../plugins/asset'
import { getDepsOptimizer } from '../optimizer'
import { applySourcemapIgnoreList, injectSourcesContent } from './sourcemap'
import { isFileServingAllowed } from './middlewares/static'
import { throwClosedServerError } from './pluginContainer'

export const ERR_LOAD_URL = 'ERR_LOAD_URL'
export const ERR_LOAD_PUBLIC_URL = 'ERR_LOAD_PUBLIC_URL'
Expand All @@ -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.


const cacheKey = (options.ssr ? 'ssr:' : options.html ? 'html:' : '') + url

// This module may get invalidated while we are processing it. For example
Expand Down Expand Up @@ -108,9 +111,8 @@ export function transformRequest(
timestamp,
abort: clearCache,
})
request.then(clearCache, clearCache)

return request
return request.finally(clearCache)
}

async function doTransform(
Expand Down Expand Up @@ -253,6 +255,9 @@ async function loadAndTransform(
err.code = isPublicFile ? ERR_LOAD_PUBLIC_URL : ERR_LOAD_URL
throw err
}

if (server._restartPromise) throwClosedServerError()

// ensure module in graph after successful load
mod ??= await moduleGraph._ensureEntryFromUrl(url, ssr, undefined, resolved)
ensureWatchedFile(watcher, mod.file, root)
Expand Down Expand Up @@ -314,6 +319,8 @@ async function loadAndTransform(
}
}

if (server._restartPromise) throwClosedServerError()

const result =
ssr && !server.config.experimental.skipSsrTransform
? await server.ssrTransform(code, map as SourceMap, url, originalCode)
Expand Down