From 24948a4e0ca50b97480664349d6df031b57ddb97 Mon Sep 17 00:00:00 2001 From: Nick Santos Date: Thu, 6 Aug 2015 23:48:39 -0400 Subject: [PATCH] More robust handling of errors moving across virtual drives Discovered while investigating https://github.com/Medium/phantomjs/issues/341 where moving a directory across virtual drives caused an EXDEV error that shunted things down the "move file" instead of "move directory" codepath --- lib/move/__tests__/move.test.js | 98 ++++++++++++++++++++++++++------- lib/move/index.js | 25 ++++++--- 2 files changed, 97 insertions(+), 26 deletions(-) diff --git a/lib/move/__tests__/move.test.js b/lib/move/__tests__/move.test.js index 21be2e9f..df6c8e87 100644 --- a/lib/move/__tests__/move.test.js +++ b/lib/move/__tests__/move.test.js @@ -2,7 +2,7 @@ var assert = require('assert') var os = require('os') var path = require('path') var rimraf = require('rimraf') -var fs = require('fs') +var fs = require('graceful-fs') var fse = require(process.cwd()) /* global afterEach, beforeEach, describe, it */ @@ -10,14 +10,31 @@ var fse = require(process.cwd()) var FIXTURES_DIR = '' var SRC_FIXTURES_DIR = path.join(__dirname, './fixtures') -// makes fs.rename return cross-device error. -var mock_fs = {} -mock_fs.rename = function (src, dest, callback) { - setTimeout(function () { - var err = new Error() - err.code = 'EXDEV' - callback(err) - }, 10) +function createAsyncErrFn (errCode) { + var fn = function () { + fn.callCount++ + var callback = arguments[arguments.length - 1] + setTimeout(function () { + var err = new Error() + err.code = errCode + callback(err) + }, 10) + } + fn.callCount = 0 + return fn +} + +var originalRename = fs.rename +var originalLink = fs.link + +function setUpMockFs (errCode) { + fs.rename = createAsyncErrFn(errCode) + fs.link = createAsyncErrFn(errCode) +} + +function tearDownMockFs () { + fs.rename = originalRename + fs.link = originalLink } describe('move', function () { @@ -97,18 +114,17 @@ describe('move', function () { var src = FIXTURES_DIR + '/a-file' var dest = FIXTURES_DIR + '/a-file-dest' - var oldRename = fs.rename - fs.rename = mock_fs.rename + setUpMockFs('EXDEV') fse.move(src, dest, function (err) { assert.ifError(err) + assert.strictEqual(fs.link.callCount, 1) + fs.readFile(dest, 'utf8', function (err, contents) { assert.ifError(err) assert.strictEqual(contents, 'sonic the hedgehog\n') - // restore - fs.rename = oldRename - + tearDownMockFs() done() }) }) @@ -131,21 +147,65 @@ describe('move', function () { }) }) - it('should move folders across devices', function (done) { + it('should move folders across devices with EISDIR erro', function (done) { + var src = FIXTURES_DIR + '/a-folder' + var dest = FIXTURES_DIR + '/a-folder-dest' + + setUpMockFs('EISDIR') + + fse.move(src, dest, function (err) { + assert.ifError(err) + assert.strictEqual(fs.link.callCount, 1) + + fs.readFile(dest + '/another-folder/file3', 'utf8', function (err, contents) { + assert.ifError(err) + assert.strictEqual(contents, 'knuckles\n') + + tearDownMockFs('EISDIR') + + done() + }) + }) + }) + + it('should clobber folders across devices', function (done) { var src = FIXTURES_DIR + '/a-folder' var dest = FIXTURES_DIR + '/a-folder-dest' - var oldRename = fs.rename - fs.rename = mock_fs.rename + fs.mkdirSync(dest) + + setUpMockFs('EXDEV') + + fse.move(src, dest, {clobber: true}, function (err) { + assert.ifError(err) + assert.strictEqual(fs.rename.callCount, 1) + + fs.readFile(dest + '/another-folder/file3', 'utf8', function (err, contents) { + assert.ifError(err) + assert.strictEqual(contents, 'knuckles\n') + + tearDownMockFs('EXDEV') + + done() + }) + }) + }) + + it('should move folders across devices with EXDEV error', function (done) { + var src = FIXTURES_DIR + '/a-folder' + var dest = FIXTURES_DIR + '/a-folder-dest' + + setUpMockFs('EXDEV') fse.move(src, dest, function (err) { assert.ifError(err) + assert.strictEqual(fs.link.callCount, 1) + fs.readFile(dest + '/another-folder/file3', 'utf8', function (err, contents) { assert.ifError(err) assert.strictEqual(contents, 'knuckles\n') - // restore - fs.rename = oldRename + tearDownMockFs() done() }) diff --git a/lib/move/index.js b/lib/move/index.js index f984c5b3..f28152f1 100644 --- a/lib/move/index.js +++ b/lib/move/index.js @@ -61,17 +61,13 @@ function mv (source, dest, options, callback) { } if (err.code !== 'EXDEV') return callback(err) - moveFileAcrossDevice(source, dest, clobber, limit, callback) + moveAcrossDevice(source, dest, clobber, limit, callback) }) } else { fs.link(source, dest, function (err) { if (err) { - if (err.code === 'EXDEV') { - moveFileAcrossDevice(source, dest, clobber, limit, callback) - return - } - if (err.code === 'EISDIR' || err.code === 'EPERM') { - moveDirAcrossDevice(source, dest, clobber, limit, callback) + if (err.code === 'EXDEV' || err.code === 'EISDIR' || err.code === 'EPERM') { + moveAcrossDevice(source, dest, clobber, limit, callback) return } callback(err) @@ -83,6 +79,21 @@ function mv (source, dest, options, callback) { } } +function moveAcrossDevice (source, dest, clobber, limit, callback) { + fs.stat(source, function (err, stat) { + if (err) { + callback(err) + return + } + + if (stat.isDirectory()) { + moveDirAcrossDevice(source, dest, clobber, limit, callback) + } else { + moveFileAcrossDevice(source, dest, clobber, limit, callback) + } + }) +} + function moveFileAcrossDevice (source, dest, clobber, limit, callback) { var outFlags = clobber ? 'w' : 'wx' var ins = fs.createReadStream(source)