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

Retry after rename EPERM error #119

Merged
merged 1 commit into from
Dec 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions lib/addRemove.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ function downloadAndExtractAsync(version) {
childNames.forEach(childName => {
let oldPath = path.join(extractedDirPath, childName);
let newPath = path.join(targetDir, childName);
fs.renameSync(oldPath, newPath);
renameWithRetry(oldPath, newPath);
});

// Remove the now-empty directory.
Expand Down Expand Up @@ -215,6 +215,38 @@ function remove(version) {
return result;
}

/**
* Synchronously renames a file or directory, retrying to handle
* occasional 'EPERM' or 'EACCS' errors.
*/
function renameWithRetry(from, to) {
// Drived/simplified from https://github.com/isaacs/node-graceful-fs/pull/119
let backoff = 0;
const backoffUntil = Date.now() + 5000;
function tryRename() {
try {
fs.renameSync(from, to);
} catch (e) {
if (!isWindows) {
// The retry with backoff is only applicable to Windows.
throw e;
} else if ((e.code === 'EACCS' || e.code === 'EPERM') && Date.now() < backoffUntil) {
if (backoff < 100) {
backoff += 10;
}
const waitUntil = Date.now() + backoff;
while (Date.now() < waitUntil) {}
tryRename();
} else if (backoff > 0 && e.code === 'ENOENT') {
// The source no longer exists; assume it was renamed.
} else {
throw e;
}
}
}
tryRename();
}

/**
* Creates a hierarchy of directories as necessary.
*/
Expand Down Expand Up @@ -286,10 +318,9 @@ async function fixNpmCmdShimsAsync(targetDir) {
if (!isWindows) return;

try {
// This assumes the npm version carried with the node installation
// includes a `cmd-shim` module. Currently true for at least npm >= 3.
const cmdShimPath = path.join(
targetDir, 'node_modules', 'npm', 'node_modules', 'cmd-shim');
fs.statSync(cmdShimPath);
const cmdShim = require(cmdShimPath);

// Enumerate .cmd files in the target directory and fix if they are shims.
Expand Down Expand Up @@ -317,6 +348,12 @@ async function fixNpmCmdShimsAsync(targetDir) {
});
}
} catch (e) {
if (e.code === 'ENOENT') {
// Currently all npm >= 3 include the cmd-shim module, but maybe
// someday it won't? Also it does not exist with test mocking.
return;
}

// Not a fatal error. Most things still work if the shims are not fixed.
// The only problem may be that the global npm package cannot be upgraded.
console.warn('Warning: Failed to fix npm cmd shims: ' + e.message);
Expand All @@ -326,4 +363,5 @@ async function fixNpmCmdShimsAsync(targetDir) {
module.exports = {
addAsync,
remove,
renameWithRetry,
};
7 changes: 7 additions & 0 deletions test/mocks/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const mockFs = {
statMap: {},
dataMap: {},
unlinkPaths: [],
nextRenameError: null,

reset() {
this.trace = false;
Expand Down Expand Up @@ -183,6 +184,12 @@ const mockFs = {
newPath = this.fixSep(newPath);
if (this.trace) console.log('renameSync(' + oldPath, newPath + ')');

if (this.nextRenameError) {
const e = this.nextRenameError;
this.nextRenameError = null;
throw e;
}

if (this.dirMap[oldPath]) {
// Support for renaming directories is limited to a single level.
// Subdirectory paths currently do not get updated.
Expand Down
7 changes: 7 additions & 0 deletions test/modules/addRemoveTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ const test = require('ava').test;
const rewire = require('rewire');
const Error = require('../../lib/error');

const isWindows = process.platform === 'win32';

test.before(require('../checkNodeVersion'));

const testHome = '/home/test/nvs/'.replace(/\//g, path.sep);
Expand Down Expand Up @@ -157,6 +159,11 @@ test('Add - download', t => {
},
});

if (isWindows) {
// Simulate occasional EPERM during rename, which should be handled by a retry.
mockFs.nextRenameError = new Error('Test rename error', 'EPERM');
}

return nvsAddRemove.addAsync(version).then(message => {
t.regex(message[0], /^Added at/);
t.truthy(nvsUse.getVersionBinary(version));
Expand Down