From a46c5afa608b88fed85d152530b22383b51bf416 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 3 Oct 2023 18:19:10 -0400 Subject: [PATCH] fix: use `fs.existsSync` to avoid race condition (#56387) Using `await fs.access` has couple of downsides. It creates unnecessary async contexts where async scope can be removed. Also, it creates the possibility of race conditions such as `Time-of-Check to Time-of-Use`. It would be nice if someone can benchmark this. I'm rooting for a performance improvement. Some updates from Node.js land: - There is an open pull request to add V8 Fast API to `existsSync` method - https://github.com/nodejs/node/pull/49893 - Non-existing `existsSync` executions became 30% faster - https://github.com/nodejs/node/pull/49593 Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --- packages/next/src/build/entries.ts | 4 +-- .../next/src/build/get-babel-config-file.ts | 8 ++--- packages/next/src/build/index.ts | 12 +++---- packages/next/src/build/load-jsconfig.ts | 8 ++--- packages/next/src/build/webpack-config.ts | 2 +- .../loaders/next-metadata-image-loader.ts | 5 ++- packages/next/src/export/index.ts | 17 +++++----- packages/next/src/lib/download-swc.ts | 14 ++++---- packages/next/src/lib/file-exists.ts | 7 ++-- .../src/lib/has-necessary-dependencies.ts | 5 ++- .../src/lib/helpers/get-cache-directory.ts | 9 ++--- packages/next/src/lib/mkcert.ts | 2 +- .../src/lib/typescript/getTypeScriptIntent.ts | 5 ++- .../src/server/dev/hot-reloader-webpack.ts | 8 ++--- .../next/src/server/dev/next-dev-server.ts | 4 +-- .../next/src/telemetry/events/swc-plugins.ts | 34 +++++++++---------- 16 files changed, 63 insertions(+), 81 deletions(-) diff --git a/packages/next/src/build/entries.ts b/packages/next/src/build/entries.ts index 4c2671cd1872a..301f7843d0481 100644 --- a/packages/next/src/build/entries.ts +++ b/packages/next/src/build/entries.ts @@ -15,6 +15,7 @@ import type { AppLoaderOptions } from './webpack/loaders/next-app-loader' import { cyan } from '../lib/picocolors' import { posix, join, dirname, extname } from 'path' import { stringify } from 'querystring' +import fs from 'fs' import { PAGES_DIR_ALIAS, ROOT_DIR_ALIAS, @@ -51,7 +52,6 @@ import { encodeMatchers } from './webpack/loaders/next-middleware-loader' import { EdgeFunctionLoaderOptions } from './webpack/loaders/next-edge-function-loader' import { isAppRouteRoute } from '../lib/is-app-route-route' import { normalizeMetadataRoute } from '../lib/metadata/get-metadata-route' -import { fileExists } from '../lib/file-exists' import { getRouteLoaderEntry } from './webpack/loaders/next-route-loader' import { isInternalComponent, @@ -123,7 +123,7 @@ export async function getStaticInfoIncludingLayouts({ while (dir.startsWith(appDir)) { for (const potentialLayoutFile of potentialLayoutFiles) { const layoutFile = join(dir, potentialLayoutFile) - if (!(await fileExists(layoutFile))) { + if (!fs.existsSync(layoutFile)) { continue } layoutFiles.unshift(layoutFile) diff --git a/packages/next/src/build/get-babel-config-file.ts b/packages/next/src/build/get-babel-config-file.ts index beb33a1ad3190..7cdbd043b7e80 100644 --- a/packages/next/src/build/get-babel-config-file.ts +++ b/packages/next/src/build/get-babel-config-file.ts @@ -1,5 +1,5 @@ -import { fileExists } from '../lib/file-exists' import { join } from 'path' +import { existsSync } from 'fs' const BABEL_CONFIG_FILES = [ '.babelrc', @@ -13,12 +13,10 @@ const BABEL_CONFIG_FILES = [ 'babel.config.cjs', ] -export async function getBabelConfigFile( - dir: string -): Promise { +export function getBabelConfigFile(dir: string): string | undefined { for (const filename of BABEL_CONFIG_FILES) { const configFilePath = join(dir, filename) - const exists = await fileExists(configFilePath) + const exists = existsSync(configFilePath) if (!exists) { continue } diff --git a/packages/next/src/build/index.ts b/packages/next/src/build/index.ts index b53a8a4794dfe..a226078d05853 100644 --- a/packages/next/src/build/index.ts +++ b/packages/next/src/build/index.ts @@ -12,7 +12,7 @@ import { loadEnvConfig } from '@next/env' import { bold, yellow, green } from '../lib/picocolors' import crypto from 'crypto' import { isMatch, makeRe } from 'next/dist/compiled/micromatch' -import fs from 'fs/promises' +import { existsSync, promises as fs } from 'fs' import os from 'os' import { Worker } from '../lib/worker' import { defaultConfig } from '../server/config-shared' @@ -408,7 +408,7 @@ export default async function build( const cacheDir = path.join(distDir, 'cache') if (ciEnvironment.isCI && !ciEnvironment.hasNextSupport) { - const hasCache = await fileExists(cacheDir) + const hasCache = existsSync(cacheDir) if (!hasCache) { // Intentionally not piping to stderr in case people fail in CI when @@ -433,7 +433,7 @@ export default async function build( const isSrcDir = path .relative(dir, pagesDir || appDir || '') .startsWith('src') - const hasPublicDir = await fileExists(publicDir) + const hasPublicDir = existsSync(publicDir) telemetry.record( eventCliSession(dir, config, { @@ -719,7 +719,7 @@ export default async function build( mappedPages['/_error'].startsWith(PAGES_DIR_ALIAS) if (hasPublicDir) { - const hasPublicUnderScoreNextDir = await fileExists( + const hasPublicUnderScoreNextDir = existsSync( path.join(publicDir, '_next') ) if (hasPublicUnderScoreNextDir) { @@ -2426,7 +2426,7 @@ export default async function build( .join('pages', '404.html') .replace(/\\/g, '/') - if (await fileExists(orig)) { + if (existsSync(orig)) { await fs.copyFile( orig, path.join(distDir, 'server', updatedRelativeDest) @@ -2901,7 +2901,7 @@ export default async function build( SERVER_DIRECTORY, 'app' ) - if (await fileExists(originalServerApp)) { + if (existsSync(originalServerApp)) { await recursiveCopy( originalServerApp, path.join( diff --git a/packages/next/src/build/load-jsconfig.ts b/packages/next/src/build/load-jsconfig.ts index e86824c84d30c..aaca3d52f7061 100644 --- a/packages/next/src/build/load-jsconfig.ts +++ b/packages/next/src/build/load-jsconfig.ts @@ -1,5 +1,5 @@ import path from 'path' -import { fileExists } from '../lib/file-exists' +import fs from 'fs' import { NextConfigComplete } from '../server/config-shared' import * as Log from './output/log' import { getTypeScriptConfiguration } from '../lib/typescript/getTypeScriptConfiguration' @@ -53,9 +53,7 @@ export default async function loadJsConfig( typeScriptPath = deps.resolved.get('typescript') } catch {} const tsConfigPath = path.join(dir, config.typescript.tsconfigPath) - const useTypeScript = Boolean( - typeScriptPath && (await fileExists(tsConfigPath)) - ) + const useTypeScript = Boolean(typeScriptPath && fs.existsSync(tsConfigPath)) let implicitBaseurl let jsConfig: { compilerOptions: Record } | undefined @@ -78,7 +76,7 @@ export default async function loadJsConfig( } const jsConfigPath = path.join(dir, 'jsconfig.json') - if (!useTypeScript && (await fileExists(jsConfigPath))) { + if (!useTypeScript && fs.existsSync(jsConfigPath)) { jsConfig = parseJsonFile(jsConfigPath) implicitBaseurl = path.dirname(jsConfigPath) } diff --git a/packages/next/src/build/webpack-config.ts b/packages/next/src/build/webpack-config.ts index 692183e05b74e..2c417158370ab 100644 --- a/packages/next/src/build/webpack-config.ts +++ b/packages/next/src/build/webpack-config.ts @@ -496,7 +496,7 @@ export default async function getBaseWebpackConfig( ? '-experimental' : '' - const babelConfigFile = await getBabelConfigFile(dir) + const babelConfigFile = getBabelConfigFile(dir) const distDir = path.join(dir, config.distDir) let useSWCLoader = !babelConfigFile || config.experimental.forceSwcTransforms diff --git a/packages/next/src/build/webpack/loaders/next-metadata-image-loader.ts b/packages/next/src/build/webpack/loaders/next-metadata-image-loader.ts index 214b4abf26d94..3df43eac07db8 100644 --- a/packages/next/src/build/webpack/loaders/next-metadata-image-loader.ts +++ b/packages/next/src/build/webpack/loaders/next-metadata-image-loader.ts @@ -7,12 +7,11 @@ import type { MetadataImageModule, PossibleImageFileNameConvention, } from './metadata/types' -import fs from 'fs/promises' +import { existsSync, promises as fs } from 'fs' import path from 'path' import loaderUtils from 'next/dist/compiled/loader-utils3' import { getImageSize } from '../../../server/image-optimizer' import { imageExtMimeTypeMap } from '../../../lib/mime-type' -import { fileExists } from '../../../lib/file-exists' import { WEBPACK_RESOURCE_QUERIES } from '../../../lib/constants' import { normalizePathSep } from '../../../shared/lib/page-path/normalize-path-sep' @@ -181,7 +180,7 @@ async function nextMetadataImageLoader(this: any, content: Buffer) { fileNameBase + '.alt.txt' ) - if (await fileExists(altPath)) { + if (existsSync(altPath)) { imageData.alt = await fs.readFile(altPath, 'utf8') } } diff --git a/packages/next/src/export/index.ts b/packages/next/src/export/index.ts index 473999c9c776f..b57f8500b0d7f 100644 --- a/packages/next/src/export/index.ts +++ b/packages/next/src/export/index.ts @@ -9,7 +9,7 @@ import type { PagesManifest } from '../build/webpack/plugins/pages-manifest-plug import { bold, yellow } from '../lib/picocolors' import findUp from 'next/dist/compiled/find-up' -import fs from 'fs/promises' +import { existsSync, promises as fs } from 'fs' import '../server/require-hook' @@ -52,7 +52,6 @@ import { isAppRouteRoute } from '../lib/is-app-route-route' import { isAppPageRoute } from '../lib/is-app-page-route' import isError from '../lib/is-error' import { needsExperimentalReact } from '../lib/needs-experimental-react' -import { fileExists } from '../lib/file-exists' import { formatManifest } from '../build/manifests/formatter/format-manifest' function divideSegments(number: number, segments: number): number[] { @@ -240,7 +239,7 @@ export async function exportAppImpl( ) return null } - if (await fileExists(join(distDir, 'server', 'app'))) { + if (existsSync(join(distDir, 'server', 'app'))) { throw new ExportError( '"next export" does not work with App Router. Please use "output: export" in next.config.js https://nextjs.org/docs/advanced-features/static-html-export' ) @@ -284,7 +283,7 @@ export async function exportAppImpl( const buildIdFile = join(distDir, BUILD_ID_FILE) - if (!(await fileExists(buildIdFile))) { + if (!existsSync(buildIdFile)) { throw new ExportError( `Could not find a production build in the '${distDir}' directory. Try building your app with 'next build' before starting the static export. https://nextjs.org/docs/messages/next-export-no-build-id` ) @@ -407,7 +406,7 @@ export async function exportAppImpl( ) // Copy static directory - if (!options.buildExport && (await fileExists(join(dir, 'static')))) { + if (!options.buildExport && existsSync(join(dir, 'static'))) { if (!options.silent) { Log.info('Copying "static" directory') } @@ -421,7 +420,7 @@ export async function exportAppImpl( // Copy .next/static directory if ( !options.buildExport && - (await fileExists(join(distDir, CLIENT_STATIC_FILES_PATH))) + existsSync(join(distDir, CLIENT_STATIC_FILES_PATH)) ) { if (!options.silent) { Log.info('Copying "static build" directory') @@ -664,7 +663,7 @@ export async function exportAppImpl( const publicDir = join(dir, CLIENT_PUBLIC_FILES_PATH) // Copy public directory - if (!options.buildExport && (await fileExists(publicDir))) { + if (!options.buildExport && existsSync(publicDir)) { if (!options.silent) { Log.info('Copying "public" directory') } @@ -818,7 +817,7 @@ export async function exportAppImpl( const handlerSrc = `${orig}.body` const handlerDest = join(outDir, route) - if (isAppRouteHandler && (await fileExists(handlerSrc))) { + if (isAppRouteHandler && existsSync(handlerSrc)) { await fs.mkdir(dirname(handlerDest), { recursive: true }) await fs.copyFile(handlerSrc, handlerDest) return @@ -852,7 +851,7 @@ export async function exportAppImpl( await fs.copyFile(htmlSrc, htmlDest) await fs.copyFile(jsonSrc, jsonDest) - if (await fileExists(`${orig}.amp.html`)) { + if (existsSync(`${orig}.amp.html`)) { await fs.mkdir(dirname(ampHtmlDest), { recursive: true }) await fs.copyFile(`${orig}.amp.html`, ampHtmlDest) } diff --git a/packages/next/src/lib/download-swc.ts b/packages/next/src/lib/download-swc.ts index 2a75653027271..7c34a4ed11a4a 100644 --- a/packages/next/src/lib/download-swc.ts +++ b/packages/next/src/lib/download-swc.ts @@ -8,7 +8,6 @@ const { fetch } = require('next/dist/compiled/undici') as { const { WritableStream } = require('node:stream/web') as { WritableStream: typeof global.WritableStream } -import { fileExists } from './file-exists' import { getRegistry } from './helpers/get-registry' import { getCacheDirectory } from './helpers/get-cache-directory' @@ -19,20 +18,19 @@ async function extractBinary( pkgName: string, tarFileName: string ) { - const cacheDirectory = await getCacheDirectory( + const cacheDirectory = getCacheDirectory( 'next-swc', process.env['NEXT_SWC_PATH'] ) - const extractFromTar = async () => { - await tar.x({ + const extractFromTar = () => + tar.x({ file: path.join(cacheDirectory, tarFileName), cwd: outputDirectory, strip: 1, }) - } - if (!(await fileExists(path.join(cacheDirectory, tarFileName)))) { + if (!fs.existsSync(path.join(cacheDirectory, tarFileName))) { Log.info(`Downloading swc package ${pkgName}...`) await fs.promises.mkdir(cacheDirectory, { recursive: true }) const tempFile = path.join( @@ -99,7 +97,7 @@ export async function downloadNativeNextSwc( const tarFileName = `${pkgName.substring(6)}-${version}.tgz` const outputDirectory = path.join(bindingsDirectory, pkgName) - if (await fileExists(outputDirectory)) { + if (fs.existsSync(outputDirectory)) { // if the package is already downloaded a different // failure occurred than not being present return @@ -119,7 +117,7 @@ export async function downloadWasmSwc( const tarFileName = `${pkgName.substring(6)}-${version}.tgz` const outputDirectory = path.join(wasmDirectory, pkgName) - if (await fileExists(outputDirectory)) { + if (fs.existsSync(outputDirectory)) { // if the package is already downloaded a different // failure occurred than not being present return diff --git a/packages/next/src/lib/file-exists.ts b/packages/next/src/lib/file-exists.ts index fff15e984794a..6d41dc3af916f 100644 --- a/packages/next/src/lib/file-exists.ts +++ b/packages/next/src/lib/file-exists.ts @@ -1,4 +1,4 @@ -import { constants, promises } from 'fs' +import { existsSync, promises } from 'fs' import isError from './is-error' export enum FileType { @@ -17,10 +17,9 @@ export async function fileExists( } else if (type === FileType.Directory) { const stats = await promises.stat(fileName) return stats.isDirectory() - } else { - await promises.access(fileName, constants.F_OK) } - return true + + return existsSync(fileName) } catch (err) { if ( isError(err) && diff --git a/packages/next/src/lib/has-necessary-dependencies.ts b/packages/next/src/lib/has-necessary-dependencies.ts index 5b81a859d2acf..271573faad441 100644 --- a/packages/next/src/lib/has-necessary-dependencies.ts +++ b/packages/next/src/lib/has-necessary-dependencies.ts @@ -1,5 +1,4 @@ -import { promises as fs } from 'fs' -import { fileExists } from './file-exists' +import { existsSync, promises as fs } from 'fs' import { resolveFrom } from './resolve-from' import { dirname, join, relative } from 'path' @@ -33,7 +32,7 @@ export async function hasNecessaryDependencies( const fileNameToVerify = relative(p.pkg, p.file) if (fileNameToVerify) { const fileToVerify = join(pkgDir, fileNameToVerify) - if (await fileExists(fileToVerify)) { + if (existsSync(fileToVerify)) { resolutions.set(p.pkg, fileToVerify) } else { return missingPackages.push(p) diff --git a/packages/next/src/lib/helpers/get-cache-directory.ts b/packages/next/src/lib/helpers/get-cache-directory.ts index fc15bb1a70dc0..339daf07c43d8 100644 --- a/packages/next/src/lib/helpers/get-cache-directory.ts +++ b/packages/next/src/lib/helpers/get-cache-directory.ts @@ -1,13 +1,10 @@ import os from 'os' import path from 'path' -import { fileExists } from '../file-exists' +import fs from 'fs' // get platform specific cache directory adapted from playwright's handling // https://github.com/microsoft/playwright/blob/7d924470d397975a74a19184c136b3573a974e13/packages/playwright-core/src/utils/registry.ts#L141 -export async function getCacheDirectory( - fileDirectory: string, - envPath?: string -) { +export function getCacheDirectory(fileDirectory: string, envPath?: string) { let result if (envPath) { @@ -29,7 +26,7 @@ export async function getCacheDirectory( path.join(os.homedir(), '.cache'), path.join(os.tmpdir()), ]) { - if (await fileExists(dir)) { + if (fs.existsSync(dir)) { systemCacheDirectory = dir break } diff --git a/packages/next/src/lib/mkcert.ts b/packages/next/src/lib/mkcert.ts index 98898cb39ea21..22897885843dd 100644 --- a/packages/next/src/lib/mkcert.ts +++ b/packages/next/src/lib/mkcert.ts @@ -36,7 +36,7 @@ function getBinaryName() { async function downloadBinary() { try { const binaryName = getBinaryName() - const cacheDirectory = await getCacheDirectory('mkcert') + const cacheDirectory = getCacheDirectory('mkcert') const binaryPath = path.join(cacheDirectory, binaryName) if (fs.existsSync(binaryPath)) { diff --git a/packages/next/src/lib/typescript/getTypeScriptIntent.ts b/packages/next/src/lib/typescript/getTypeScriptIntent.ts index 919274352f47a..f66bb7f88683f 100644 --- a/packages/next/src/lib/typescript/getTypeScriptIntent.ts +++ b/packages/next/src/lib/typescript/getTypeScriptIntent.ts @@ -1,6 +1,5 @@ -import { promises as fs } from 'fs' +import { existsSync, promises as fs } from 'fs' import path from 'path' -import { fileExists } from '../file-exists' import { recursiveReadDir } from '../recursive-readdir' export type TypeScriptIntent = { firstTimeSetup: boolean } @@ -14,7 +13,7 @@ export async function getTypeScriptIntent( // The integration turns on if we find a `tsconfig.json` in the user's // project. - const hasTypeScriptConfiguration = await fileExists(resolvedTsConfigPath) + const hasTypeScriptConfiguration = existsSync(resolvedTsConfigPath) if (hasTypeScriptConfiguration) { const content = await fs .readFile(resolvedTsConfigPath, { encoding: 'utf8' }) diff --git a/packages/next/src/server/dev/hot-reloader-webpack.ts b/packages/next/src/server/dev/hot-reloader-webpack.ts index 2578b40cba60c..20fa94826f3a1 100644 --- a/packages/next/src/server/dev/hot-reloader-webpack.ts +++ b/packages/next/src/server/dev/hot-reloader-webpack.ts @@ -48,7 +48,6 @@ import { import { denormalizePagePath } from '../../shared/lib/page-path/denormalize-page-path' import { normalizePathSep } from '../../shared/lib/page-path/normalize-path-sep' import getRouteFromEntrypoint from '../get-route-from-entrypoint' -import { fileExists } from '../../lib/file-exists' import { difference, isMiddlewareFile, @@ -58,7 +57,7 @@ import { DecodeError } from '../../shared/lib/utils' import { Span, trace } from '../../trace' import { getProperError } from '../../lib/is-error' import ws from 'next/dist/compiled/ws' -import { promises as fs } from 'fs' +import { existsSync, promises as fs } from 'fs' import { UnwrapPromise } from '../../lib/coalesced-function' import { getRegistry } from '../../lib/helpers/get-registry' import { RouteMatch } from '../future/route-matches/route-match' @@ -768,7 +767,7 @@ export default class HotReloader implements NextJsHotReloaderInterface { // Check if the page was removed or disposed and remove it if (isEntry) { const pageExists = - !dispose && (await fileExists(entryData.absolutePagePath)) + !dispose && existsSync(entryData.absolutePagePath) if (!pageExists) { delete entries[entryKey] return @@ -779,8 +778,7 @@ export default class HotReloader implements NextJsHotReloaderInterface { if (isChildEntry) { if (entryData.absoluteEntryFilePath) { const pageExists = - !dispose && - (await fileExists(entryData.absoluteEntryFilePath)) + !dispose && existsSync(entryData.absoluteEntryFilePath) if (!pageExists) { delete entries[entryKey] return diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index 2928278686ca0..937e303461db4 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -15,6 +15,7 @@ import type { NextUrlWithParsedQuery, } from '../request-meta' +import fs from 'fs' import { Worker } from 'next/dist/compiled/jest-worker' import { join as pathJoin } from 'path' import { ampValidation } from '../../build/output' @@ -22,7 +23,6 @@ import { INSTRUMENTATION_HOOK_FILENAME, PUBLIC_DIR_MIDDLEWARE_CONFLICT, } from '../../lib/constants' -import { fileExists } from '../../lib/file-exists' import { findPagesDir } from '../../lib/find-pages-dir' import { PHASE_DEVELOPMENT_SERVER, @@ -453,7 +453,7 @@ export default class DevServer extends Server { const { pathname } = parsedUrl if (pathname!.startsWith('/_next')) { - if (await fileExists(pathJoin(this.publicDir, '_next'))) { + if (fs.existsSync(pathJoin(this.publicDir, '_next'))) { throw new Error(PUBLIC_DIR_MIDDLEWARE_CONFLICT) } } diff --git a/packages/next/src/telemetry/events/swc-plugins.ts b/packages/next/src/telemetry/events/swc-plugins.ts index 157799041ce81..c0105d64952a6 100644 --- a/packages/next/src/telemetry/events/swc-plugins.ts +++ b/packages/next/src/telemetry/events/swc-plugins.ts @@ -1,6 +1,6 @@ import findUp from 'next/dist/compiled/find-up' import path from 'path' -import { fileExists } from '../../lib/file-exists' +import fs from 'fs' import type { NextConfig } from '../../server/config-shared' const EVENT_SWC_PLUGIN_PRESENT = 'NEXT_SWC_PLUGIN_DETECTED' @@ -28,24 +28,22 @@ export async function eventSwcPlugins( const swcPluginPackages = config.experimental?.swcPlugins?.map(([name, _]) => name) ?? [] - return Promise.all( - swcPluginPackages.map(async (plugin) => { - // swc plugins can be non-npm pkgs with absolute path doesn't have version - const version = deps[plugin] ?? undefined - let pluginName = plugin - if (await fileExists(pluginName)) { - pluginName = path.basename(plugin, '.wasm') - } + return swcPluginPackages.map((plugin) => { + // swc plugins can be non-npm pkgs with absolute path doesn't have version + const version = deps[plugin] ?? undefined + let pluginName = plugin + if (fs.existsSync(pluginName)) { + pluginName = path.basename(plugin, '.wasm') + } - return { - eventName: EVENT_SWC_PLUGIN_PRESENT, - payload: { - pluginName: pluginName, - pluginVersion: version, - }, - } - }) - ) + return { + eventName: EVENT_SWC_PLUGIN_PRESENT, + payload: { + pluginName: pluginName, + pluginVersion: version, + }, + } + }) } catch (_) { return [] }