From c4e05dfffe532122a2c35c326f31b34c5a43ea57 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 12 Nov 2019 14:48:53 -0500 Subject: [PATCH 1/5] Rewrite files.rename on Windows to retry after 50ms intervals. Falling back to a full recursive copy was MUCH more expensive than waiting a short amount of time before retrying the rename. This aligns with the way graceful-fs handles EPERM and EACCES errors on Windows: https://www.npmjs.com/package/graceful-fs#improvements-over-fs-module --- tools/fs/files.ts | 49 +++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/tools/fs/files.ts b/tools/fs/files.ts index 24d57da570..5df5b6eb16 100644 --- a/tools/fs/files.ts +++ b/tools/fs/files.ts @@ -1694,29 +1694,40 @@ const wrappedRename = wrapDestructiveFsFunc("rename", fs.renameSync, [0, 1]); export const rename = isWindowsLikeFilesystem() ? function (from: string, to: string) { // Retries are necessary only on Windows, because the rename call can // fail with EBUSY, which means the file is in use. - let maxTries = 10; - let success = false; const osTo = convertToOSPath(to); + const startTimeMs = Date.now(); + const intervalMs = 50; + const timeLimitMs = 1000; - while (! success && maxTries-- > 0) { - try { - // Despite previous failures, the top-level destination directory - // may have been successfully created, so we must remove it to - // avoid moving the source file *into* the destination directory. - rimraf.sync(osTo); - wrappedRename(from, to); - success = true; - } catch (err) { - if (err.code !== 'EPERM' && err.code !== 'EACCES') { - throw err; + return new Promise((resolve, reject) => { + function attempt() { + try { + // Despite previous failures, the top-level destination directory + // may have been successfully created, so we must remove it to + // avoid moving the source file *into* the destination directory. + rimraf.sync(osTo); + wrappedRename(from, to); + resolve(); + } catch (err) { + if (err.code !== 'EPERM' && err.code !== 'EACCES') { + reject(err); + } else if (Date.now() - startTimeMs < timeLimitMs) { + setTimeout(attempt, intervalMs); + } else { + reject(err); + } } } - } - - if (! success) { - cp_r(from, to, { preserveSymlinks: true }); - rm_recursive(from); - } + attempt(); + }).catch(error => { + if (error.code === 'EPERM' || + error.code === 'EACCESS') { + cp_r(from, to, { preserveSymlinks: true }); + rm_recursive(from); + } else { + throw error; + } + }).await(); } : wrappedRename; // Warning: doesn't convert slashes in the second 'cache' arg From 8567390e7e24e08053dcb0df1975619daec560e6 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 12 Nov 2019 16:29:36 -0500 Subject: [PATCH 2/5] Fix error stack trace parsing during Windows self-tests. --- tools/tool-testing/run.js | 33 ++++++++++++++++++++++++++++++--- tools/tool-testing/sandbox.js | 2 +- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/tools/tool-testing/run.js b/tools/tool-testing/run.js index d457ffbd03..0f45f73cea 100644 --- a/tools/tool-testing/run.js +++ b/tools/tool-testing/run.js @@ -461,10 +461,37 @@ export default class Run { if (failure instanceof TestFailure) { const frames = parseStackParse(failure).outsideFiber; - const relpath = files.pathRelative(files.getCurrentToolsDir(), - frames[0].file); + const toolsDir = files.getCurrentToolsDir(); + let pathWithLineNumber; + frames.some(frame => { + // The parsed stack trace will typically include frame.file + // strings of the form "/tools/tests/whatever.js", which can be + // made absolute by joining them with toolsDir. If the resulting + // absPath exists, then we know we interpreted the frame.file + // correctly, and we can normalize away the leading '/' + // character to get a safe relative path. + const absPath = files.pathJoin(toolsDir, frame.file); + if (files.exists(absPath)) { + const relPath = files.pathRelative(toolsDir, absPath); + const parts = relPath.split("/"); + if (parts[0] === "tools" && + parts[1] === "tool-testing") { + // Ignore frames inside the /tools/tool-testing directory, + // like run.js and selftest.js. + return false; + } + pathWithLineNumber = `${relPath}:${frame.line}`; + return true; + } + // If frame.file was not joinable with toolsDir to obtain an + // absolute path that exists, show it to the user without trying + // to interpret what it means. + pathWithLineNumber = `${frame.file}:${frame.line}`; + return true; + }); + Console.rawError( - ` => ${failure.reason} at ${relpath}:${frames[0].line}\n`); + ` => ${failure.reason} at ${pathWithLineNumber}\n`); if (failure.reason === 'no-match' || failure.reason === 'junk-before' || failure.reason === 'match-timeout') { Console.arrowError(`Pattern: ${failure.details.pattern}`, 2); diff --git a/tools/tool-testing/sandbox.js b/tools/tool-testing/sandbox.js index d7b34ad820..88e753f84a 100644 --- a/tools/tool-testing/sandbox.js +++ b/tools/tool-testing/sandbox.js @@ -368,7 +368,7 @@ export default class Sandbox { // Allow user to set TOOL_NODE_FLAGS for self-test app. if (process.env.TOOL_NODE_FLAGS && ! process.env.SELF_TEST_TOOL_NODE_FLAGS) console.log('Consider setting SELF_TEST_TOOL_NODE_FLAGS to configure ' + - 'self-test test applicaion spawns'); + 'self-test test application spawns'); env.TOOL_NODE_FLAGS = process.env.SELF_TEST_TOOL_NODE_FLAGS || ''; return env; From d52c9cc82fd70d1e080aa14de5c85fe80d3e57be Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 12 Nov 2019 16:58:05 -0500 Subject: [PATCH 3/5] Avoid calling writeFileAtomically outside Fiber. --- tools/isobuild/import-scanner.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/isobuild/import-scanner.ts b/tools/isobuild/import-scanner.ts index 5fc67fd472..b4377f95dc 100644 --- a/tools/isobuild/import-scanner.ts +++ b/tools/isobuild/import-scanner.ts @@ -142,7 +142,9 @@ class DefaultHandlers { file.hash, this.bundleArch, ); - process.nextTick(writeFileAtomically, cacheFileName, code); + Promise.resolve().then( + () => writeFileAtomically(cacheFileName, code), + ); return code; } } else { From 52d4809aeb77cb636332297b0256a9bac1476858 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 12 Nov 2019 17:03:13 -0500 Subject: [PATCH 4/5] Temporarily wrap files.rename on non-Windows platforms to validate safety. Now that files.rename uses Promise.prototype.await on Windows, it's important to be sure it never gets called outside of a Fiber. Though we don't run our full test suite on Windows, we can validate this expectation by wrapping files.rename on all platforms. This commit should be reverted once the validation is complete. --- tools/fs/files.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/fs/files.ts b/tools/fs/files.ts index 5df5b6eb16..3fbc8ee1ec 100644 --- a/tools/fs/files.ts +++ b/tools/fs/files.ts @@ -35,7 +35,6 @@ import { convertToStandardLineEndings, convertToStandardPath, convertToWindowsPath, - isWindowsLikeFilesystem, pathBasename, pathDirname, pathJoin, @@ -1691,7 +1690,7 @@ export function copyFile(from: string, to: string, flags = 0) { } const wrappedRename = wrapDestructiveFsFunc("rename", fs.renameSync, [0, 1]); -export const rename = isWindowsLikeFilesystem() ? function (from: string, to: string) { +export function rename(from: string, to: string) { // Retries are necessary only on Windows, because the rename call can // fail with EBUSY, which means the file is in use. const osTo = convertToOSPath(to); @@ -1728,7 +1727,7 @@ export const rename = isWindowsLikeFilesystem() ? function (from: string, to: st throw error; } }).await(); -} : wrappedRename; +} // Warning: doesn't convert slashes in the second 'cache' arg export const realpath = From faeab6c7d49be7f4c6ecc6006937fd62edf9bcb8 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 12 Nov 2019 17:22:33 -0500 Subject: [PATCH 5/5] Revert "Temporarily wrap files.rename on non-Windows platforms to validate safety." This reverts commit 52d4809aeb77cb636332297b0256a9bac1476858, as promised in the previous commit message. --- tools/fs/files.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tools/fs/files.ts b/tools/fs/files.ts index 3fbc8ee1ec..5df5b6eb16 100644 --- a/tools/fs/files.ts +++ b/tools/fs/files.ts @@ -35,6 +35,7 @@ import { convertToStandardLineEndings, convertToStandardPath, convertToWindowsPath, + isWindowsLikeFilesystem, pathBasename, pathDirname, pathJoin, @@ -1690,7 +1691,7 @@ export function copyFile(from: string, to: string, flags = 0) { } const wrappedRename = wrapDestructiveFsFunc("rename", fs.renameSync, [0, 1]); -export function rename(from: string, to: string) { +export const rename = isWindowsLikeFilesystem() ? function (from: string, to: string) { // Retries are necessary only on Windows, because the rename call can // fail with EBUSY, which means the file is in use. const osTo = convertToOSPath(to); @@ -1727,7 +1728,7 @@ export function rename(from: string, to: string) { throw error; } }).await(); -} +} : wrappedRename; // Warning: doesn't convert slashes in the second 'cache' arg export const realpath =