Merge pull request #1077 from neoziro/fix-concurrent-renaming

Fix bug with concurrent renaming

lib/core/ResolveCache.js

@@ -108,23 +108,43 @@ ResolveCache.prototype.store = function (canonicalDir, pkgMeta) {
         release = that._getPkgRelease(pkgMeta);
         dir = path.join(that._dir, sourceId, release);
 
-        // Check if directory exists
-        return Q.nfcall(fs.stat, dir)
-        .then(function () {
-            // If it does exists, remove it
-            return Q.nfcall(rimraf, dir);
-        }, function (err) {
-            // If directory does not exists, ensure its basename
-            // is created
-            if (err.code === 'ENOENT') {
-                return Q.nfcall(mkdirp, path.dirname(dir));
-            }
+        var checkExistingDirectory;
+        var key = 'moving:' + dir;
 
-            throw err;
-        })
+        if (! that._cache.has(key)) {
+            // Check if directory exists
+            checkExistingDirectory = Q.nfcall(fs.stat, dir)
+            .then(function () {
+                // If it does exists, remove it
+                return Q.nfcall(rimraf, dir);
+            }, function (err) {
+                // If directory does not exists, ensure its basename
+                // is created
+                if (err.code === 'ENOENT') {
+                    return Q.nfcall(mkdirp, path.dirname(dir));
+                }
+
+                throw err;
+            });
+        }
+        else {
+            checkExistingDirectory = new Q();
+        }
+
+        return checkExistingDirectory
         // Move the canonical to sourceId/target
         .then(function () {
-            return Q.nfcall(fs.rename, canonicalDir, dir)
+            // We store the renaming in cache.
+            var promise;
+            if (that._cache.has(key)) {
+                promise = that._cache.get(key);
+            }
+            else {
+                promise = Q.nfcall(fs.rename, canonicalDir, dir);
+                that._cache.set(key, promise);
+            }
+
+            return promise
             .fail(function (err) {
                 // If error is EXDEV it means that we are trying to rename
                 // across different drives, so we copy and remove it instead
@@ -132,10 +152,11 @@ ResolveCache.prototype.store = function (canonicalDir, pkgMeta) {
                     throw err;
                 }
 
-                return copy.copyDir(canonicalDir, dir)
-                .then(function () {
-                    return Q.nfcall(rimraf, canonicalDir);
-                });
+                return copy.copyDir(canonicalDir, dir);
+            })
+            // To match all case, we remove the directory.
+            .then(function () {
+                return Q.nfcall(rimraf, canonicalDir);
             });
         });
     })

test/core/resolveCache.js

@@ -310,6 +310,30 @@ describe('ResolveCache', function () {
             })
             .done();
         });
+
+        it('should be possible to store two package at same time', function (next) {
+            var store = resolveCache.store.bind(resolveCache, tempPackage, {
+                name: 'foo',
+                _source: 'foo',
+                _target: 'foo/bar'
+            });
+            var store2 = resolveCache.store.bind(resolveCache, tempPackage2, {
+                name: 'foo',
+                _source: 'foo',
+                _target: 'foo/bar'
+            });
+
+            Q.all([store(), store2()]).then(function (dirs) {
+                var dir = dirs[0];
+                expect(dir).to.equal(path.join(cacheDir, md5('foo'), 'foo%2Fbar'));
+                expect(fs.existsSync(dir)).to.be(true);
+                expect(fs.existsSync(path.join(dir, 'baz'))).to.be(true);
+                expect(fs.existsSync(tempPackage)).to.be(false);
+                expect(fs.existsSync(tempPackage2)).to.be(false);
+
+                next();
+            }).done();
+        });
     });
 
     describe('.versions', function () {