diff --git a/lib/build.js b/lib/build.js index 00a0abe691..5070124930 100644 --- a/lib/build.js +++ b/lib/build.js @@ -204,7 +204,11 @@ async function build (gyp, argv) { } catch (err) { if (err.code !== 'ENOENT') throw err } - await fs.symlink(python, symlinkDestination) + try { + await fs.symlink(python, symlinkDestination) + } catch (err) { + if (err.code !== 'EEXIST') throw err + } log.verbose('bin symlinks', `created symlink to "${python}" in "${buildBinsDir}" and added to PATH`) } diff --git a/lib/copy-directory.js b/lib/copy-directory.js new file mode 100644 index 0000000000..ab25300bed --- /dev/null +++ b/lib/copy-directory.js @@ -0,0 +1,58 @@ +'use strict' + +const { promises: fs } = require('graceful-fs') +const crypto = require('crypto') +const path = require('path') + +const RACE_ERRORS = ['ENOTEMPTY', 'EEXIST', 'EBUSY', 'EPERM'] + +async function copyDirectory (src, dest) { + try { + await fs.stat(src) + } catch { + throw new Error(`Missing source directory for copy: ${src}`) + } + await fs.mkdir(dest, { recursive: true }) + const entries = await fs.readdir(src, { withFileTypes: true }) + for (const entry of entries) { + if (!entry.isDirectory() && !entry.isFile()) { + throw new Error('Unexpected file directory entry type') + } + + // With parallel installs, multiple processes race to place the same + // entry. Use fs.rename for an atomic move so no process ever sees a + // partially written file. For cross-filesystem (EXDEV), copy to a + // temp path in the dest directory first, then rename within the + // same filesystem to keep it atomic. + // + // When another process wins the race, rename may fail with one of + // these codes — all mean the destination was already placed and + // are safe to ignore since every process extracts identical content. + const srcPath = path.join(src, entry.name) + const destPath = path.join(dest, entry.name) + try { + await fs.rename(srcPath, destPath) + } catch (err) { + if (RACE_ERRORS.includes(err.code)) { + // Another parallel process already placed this entry — ignore + } else if (err.code === 'EXDEV') { + // Cross-filesystem: copy to a uniquely named temp path in the + // dest directory, then rename into place atomically + const tmpPath = `${destPath}.tmp.${crypto.randomBytes(6).toString('hex')}` + try { + await fs.cp(srcPath, tmpPath, { recursive: true }) + await fs.rename(tmpPath, destPath) + } catch (e) { + await fs.rm(tmpPath, { recursive: true, force: true }).catch(() => {}) + if (!RACE_ERRORS.includes(e.code)) { + throw e + } + } + } else { + throw err + } + } + } +} + +module.exports = copyDirectory diff --git a/lib/install.js b/lib/install.js index 3580cdd003..9bedbb3876 100644 --- a/lib/install.js +++ b/lib/install.js @@ -2,13 +2,13 @@ const { createWriteStream, promises: fs } = require('graceful-fs') const os = require('os') -const { backOff } = require('exponential-backoff') const tar = require('tar') const path = require('path') const { Transform, promises: { pipeline } } = require('stream') const crypto = require('crypto') const log = require('./log') const semver = require('semver') +const copyDirectory = require('./copy-directory') const { download } = require('./download') const processRelease = require('./process-release') @@ -119,40 +119,6 @@ async function install (gyp, argv) { } } - async function copyDirectory (src, dest) { - try { - await fs.stat(src) - } catch { - throw new Error(`Missing source directory for copy: ${src}`) - } - await fs.mkdir(dest, { recursive: true }) - const entries = await fs.readdir(src, { withFileTypes: true }) - for (const entry of entries) { - if (entry.isDirectory()) { - await copyDirectory(path.join(src, entry.name), path.join(dest, entry.name)) - } else if (entry.isFile()) { - // with parallel installs, copying files may cause file errors on - // Windows so use an exponential backoff to resolve collisions - await backOff(async () => { - try { - await fs.copyFile(path.join(src, entry.name), path.join(dest, entry.name)) - } catch (err) { - // if ensure, check if file already exists and that's good enough - if (gyp.opts.ensure && err.code === 'EBUSY') { - try { - await fs.stat(path.join(dest, entry.name)) - return - } catch {} - } - throw err - } - }) - } else { - throw new Error('Unexpected file directory entry type') - } - } - } - async function go () { log.verbose('ensuring devDir is created', devDir) diff --git a/package.json b/package.json index 71e0790043..c6c51d8639 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,6 @@ "main": "./lib/node-gyp.js", "dependencies": { "env-paths": "^2.2.0", - "exponential-backoff": "^3.1.1", "graceful-fs": "^4.2.6", "make-fetch-happen": "^15.0.0", "nopt": "^9.0.0", diff --git a/test/test-addon.js b/test/test-addon.js index 1f4d95ed7e..d5868a099f 100644 --- a/test/test-addon.js +++ b/test/test-addon.js @@ -1,13 +1,14 @@ 'use strict' -const { describe, it } = require('mocha') +const { describe, it, beforeEach, afterEach } = require('mocha') const assert = require('assert') const path = require('path') const fs = require('graceful-fs') +const { rm, mkdtemp } = require('fs/promises') const os = require('os') const cp = require('child_process') const util = require('../lib/util') -const { platformTimeout } = require('./common') +const { FULL_TEST, platformTimeout } = require('./common') const addonPath = path.resolve(__dirname, 'node_modules', 'hello_world') const nodeGyp = path.resolve(__dirname, '..', 'bin', 'node-gyp.js') @@ -129,4 +130,61 @@ describe('addon', function () { assert.strictEqual(runHello(notNodePath), 'world') fs.unlinkSync(notNodePath) }) + + describe('parallel', function () { + let devDir + let addonCopiesDir + + beforeEach(async () => { + devDir = await mkdtemp(path.join(os.tmpdir(), 'node-gyp-test-')) + addonCopiesDir = await mkdtemp(path.join(os.tmpdir(), 'node-gyp-test-addons-')) + }) + + afterEach(async () => { + await Promise.all([ + rm(devDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 1000 }), + rm(addonCopiesDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 1000 }) + ]) + devDir = null + addonCopiesDir = null + }) + + const runIt = (name, fn) => { + if (!FULL_TEST) { + return it.skip('Skipping parallel rebuild test due to test environment configuration') + } + + if (process.platform === 'darwin' && process.arch === 'x64') { + return it.skip('Skipping parallel rebuild test on x64 macOS') + } + + return it(name, async function () { + this.timeout(platformTimeout(4, { win32: 20 })) + await fn.call(this) + }) + } + + runIt('parallel rebuild', async function () { + // Install dependencies (nan) so copies in temp directories can resolve them + const [npmErr] = await util.execFile('npm', ['install', '--ignore-scripts'], { cwd: addonPath, shell: process.platform === 'win32' }) + assert.strictEqual(npmErr, null) + + const copies = await Promise.all(new Array(5).fill(0).map(async (_, i) => { + const copyDir = path.join(addonCopiesDir, `hello_world_${i}`) + await fs.promises.cp(addonPath, copyDir, { recursive: true }) + return copyDir + })) + await Promise.all(copies.map(async (copyDir, i) => { + const cmd = [nodeGyp, 'rebuild', '-C', copyDir, '--loglevel=verbose', `--devdir=${devDir}`] + const title = `${' '.repeat(8)}parallel rebuild ${(i + 1).toString().padEnd(2, ' ')}` + console.log(`${title} : Start`) + console.time(title) + const [err, logLines] = await execFile(cmd) + console.timeEnd(title) + const lastLine = logLines[logLines.length - 1] + assert.strictEqual(err, null) + assert.strictEqual(lastLine, 'gyp info ok', 'should end in ok') + })) + }) + }) }) diff --git a/test/test-copy-directory.js b/test/test-copy-directory.js new file mode 100644 index 0000000000..f6349153a0 --- /dev/null +++ b/test/test-copy-directory.js @@ -0,0 +1,80 @@ +'use strict' + +const { describe, it, afterEach } = require('mocha') +const assert = require('assert') +const path = require('path') +const fs = require('fs') +const { promises: fsp } = fs +const os = require('os') +const { FULL_TEST, platformTimeout } = require('./common') +const copyDirectory = require('../lib/copy-directory') + +describe('copyDirectory', function () { + let timer + let tmpDir + + afterEach(async () => { + if (tmpDir) { + await fsp.rm(tmpDir, { recursive: true, force: true }) + tmpDir = null + } + clearInterval(timer) + }) + + it('large file appears atomically (no partial writes visible)', async function () { + if (!FULL_TEST) { + return this.skip('Skipping due to test environment configuration') + } + + this.timeout(platformTimeout(5, { win32: 10 })) + + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'node-gyp-copy-test-')) + const srcDir = path.join(tmpDir, 'src') + const destDir = path.join(tmpDir, 'dest') + await fsp.mkdir(srcDir) + + const fileName = 'large.bin' + const srcFile = path.join(srcDir, fileName) + const destFile = path.join(destDir, fileName) + + // Create a 5 GB sparse file — instant to create, consumes no real + // disk, but fs.copyFile still has to process the full extent map so + // the destination file is visible at size 0 and grows over time. + // fs.rename() is atomic at the VFS level: the file either does not + // exist at the destination or appears at its full size in one step. + const fileSize = 5 * 1024 * 1024 * 1024 + const handle = await fsp.open(srcFile, 'w') + await handle.truncate(fileSize) + await handle.close() + + // Tight synchronous poll: stat the destination on every event-loop + // turn while copyDirectory runs concurrently. + let polls = 0 + const violations = [] + + timer = setInterval(() => { + try { + const stat = fs.statSync(destFile) + polls++ + if (stat.size !== fileSize) { + violations.push({ poll: polls, size: stat.size }) + } + } catch (err) { + if (err.code !== 'ENOENT') throw err + } + }, 0) + + await copyDirectory(srcDir, destDir) + + clearInterval(timer) + timer = undefined + + console.log(` ${polls} stats observed the file during the operation`) + + assert.strictEqual(violations.length, 0, 'file must never be observed at a partial size') + + const finalStat = await fsp.stat(destFile) + assert.strictEqual(finalStat.size, fileSize, + 'destination file should have the correct final size') + }) +})