fix: fix mtime precision on some filesystems

Closes #82, #87

lib/lockfile.js

@@ -4,6 +4,7 @@ const path = require('path');
 const fs = require('graceful-fs');
 const retry = require('retry');
 const onExit = require('signal-exit');
+const createMtimeChecker = require('./mtime-checker');
 
 const locks = {};
 
@@ -24,9 +25,22 @@ function resolveCanonicalPath(file, options, callback) {
 function acquireLock(file, options, callback) {
     // Use mkdir to create the lockfile (atomic operation)
     options.fs.mkdir(getLockFile(file, options), (err) => {
-        // If successful, we are done
         if (!err) {
-            return options.fs.stat(getLockFile(file, options), callback);
+            // At this point, we acquired the lock!
+            // Initialize the mtime checker and set the mtime of the lockfile
+            const mtimeChecker = createMtimeChecker();
+            const mtime = mtimeChecker.getDate();
+
+            return options.fs.utimes(getLockFile(file, options), mtime, mtime, (err) => {
+                // If it failed, try to remove the lock..
+                if (err) {
+                    options.fs.rmdir(getLockFile(file, options), () => {});
+
+                    return callback(err);
+                }
+
+                callback(null, mtime, mtimeChecker);
+            });
         }
 
         // If error is not EEXIST then some other error occurred while locking
@@ -95,7 +109,8 @@ function updateLock(file, options) {
     lock.updateTimeout = setTimeout(() => {
         lock.updateTimeout = null;
 
-        // Check if mtime is still ours if it is we can still recover from a system sleep or a busy event loop
+        // Stat the file to check if mtime is still ours
+        // If it is we can still recover from a system sleep or a busy event loop
         options.fs.stat(getLockFile(file, options), (err, stat) => {
             const isOverThreshold = lock.lastUpdate + options.stale < Date.now();
 
@@ -111,7 +126,7 @@ function updateLock(file, options) {
                 return updateLock(file, options);
             }
 
-            const isMtimeOurs = lock.mtimeChecker(lock.mtime, stat.mtime);
+            const isMtimeOurs = lock.mtimeChecker.check(lock.mtime, stat.mtime);
 
             if (!isMtimeOurs) {
                 return setLockAsCompromised(
@@ -123,7 +138,7 @@ function updateLock(file, options) {
                     ));
             }
 
-            const mtime = new Date();
+            const mtime = lock.mtimeChecker.getDate();
 
             options.fs.utimes(getLockFile(file, options), mtime, mtime, (err) => {
                 const isOverThreshold = lock.lastUpdate + options.stale < Date.now();
@@ -215,7 +230,7 @@ function lock(file, options, callback) {
         const operation = retry.operation(options.retries);
 
         operation.attempt(() => {
-            acquireLock(file, options, (err, stat) => {
+            acquireLock(file, options, (err, mtime, mtimeChecker) => {
                 if (operation.retry(err)) {
                     return;
                 }
@@ -226,10 +241,10 @@ function lock(file, options, callback) {
 
                 // We now own the lock
                 const lock = locks[file] = {
-                    mtime: stat.mtime,
+                    mtime,
+                    mtimeChecker,
                     options,
                     lastUpdate: Date.now(),
-                    mtimeChecker: createMtimeChecker(),
                 };
 
                 // We must keep the lock fresh to avoid staleness
@@ -310,29 +325,6 @@ function getLocks() {
     return locks;
 }
 
-function createMtimeChecker() {
-    let precision;
-
-    return (lockMtime, statMtime) => {
-        // If lock time was not on the second we can determine precision
-        if (!precision && lockMtime % 1000 !== 0) {
-            precision = statMtime % 1000 === 0 ? 's' : 'ms';
-        }
-
-        if (precision === 's') {
-            const lockTs = lockMtime.getTime();
-            const statTs = statMtime.getTime();
-
-            // Maybe the file system truncates or rounds...
-            return Math.trunc(lockTs / 1000) === Math.trunc(statTs / 1000) ||
-              Math.round(lockTs / 1000) === Math.round(statTs / 1000);
-        }
-
-        // Must be ms or lockMtime was on the second
-        return lockMtime.getTime() === statMtime.getTime();
-    };
-}
-
 // Remove acquired locks on exit
 /* istanbul ignore next */
 onExit(() => {

lib/mtime-checker.js

@@ -0,0 +1,50 @@
+'use strict';
+
+function getMtimeToProbe() {
+    const now = Date.now();
+
+    return new Date(now % 1000 === 0 ? now + 1 : now);
+}
+
+function maybeTruncateMtime(mtime, precision) {
+    if (precision === 'ms') {
+        return mtime;
+    }
+
+    const mtimeMs = Math.trunc(mtime.getTime() / 1000) * 1000;
+
+    return new Date(mtimeMs);
+}
+
+function calculatePrecision(probedMtime) {
+    return probedMtime.getTime() % 1000 === 0 ? 's' : 'ms';
+}
+
+function createMtimeChecker() {
+    let precision;
+
+    return {
+        check(issuedMtime, statMtime) {
+            if (!precision) {
+                precision = calculatePrecision(statMtime);
+            }
+
+            return maybeTruncateMtime(issuedMtime, precision).getTime() === maybeTruncateMtime(statMtime, precision).getTime();
+        },
+        getDate() {
+            let mtime;
+
+            // If we already calculated the precision, return Date.now() according to it
+            if (precision) {
+                mtime = maybeTruncateMtime(new Date(), precision);
+            // Otherwise, return "not on the second" Date.now() in ms in order to probe
+            } else {
+                mtime = getMtimeToProbe();
+            }
+
+            return mtime;
+        },
+    };
+}
+
+module.exports = createMtimeChecker;

test/lock.test.js

@@ -204,7 +204,7 @@ it('should retry if the lockfile was removed when verifying staleness', async ()
     await lockfile.lock(`${tmpDir}/foo`, { fs: customFs });
 
     expect(customFs.mkdir).toHaveBeenCalledTimes(2);
-    expect(customFs.stat).toHaveBeenCalledTimes(2);
+    expect(customFs.stat).toHaveBeenCalledTimes(1);
     expect(fs.statSync(`${tmpDir}/foo.lock`).mtime.getTime()).toBeGreaterThan(Date.now() - 3000);
 });
 
@@ -363,10 +363,7 @@ it('should call the compromised function if ENOENT was detected when updating th
 it('should call the compromised function if failed to update the lockfile mtime too many times', async () => {
     fs.writeFileSync(`${tmpDir}/foo`, '');
 
-    const customFs = {
-        ...fs,
-        utimes: (path, atime, mtime, callback) => callback(new Error('foo')),
-    };
+    const customFs = { ...fs };
 
     const deferred = pDefer();
 
@@ -384,16 +381,15 @@ it('should call the compromised function if failed to update the lockfile mtime
         onCompromised: handleCompromised,
     });
 
+    customFs.utimes = (path, atime, mtime, callback) => callback(new Error('foo'));
+
     await deferred.promise;
 }, 10000);
 
 it('should call the compromised function if updating the lockfile took too much time', async () => {
     fs.writeFileSync(`${tmpDir}/foo`, '');
 
-    const customFs = {
-        ...fs,
-        utimes: (path, atime, mtime, callback) => setTimeout(() => callback(new Error('foo')), 6000),
-    };
+    const customFs = { ...fs };
 
     const deferred = pDefer();
 
@@ -411,16 +407,15 @@ it('should call the compromised function if updating the lockfile took too much
         onCompromised: handleCompromised,
     });
 
+    customFs.utimes = (path, atime, mtime, callback) => setTimeout(() => callback(new Error('foo')), 6000);
+
     await deferred.promise;
 }, 10000);
 
 it('should call the compromised function if lock was acquired by someone else due to staleness', async () => {
     fs.writeFileSync(`${tmpDir}/foo`, '');
 
-    const customFs = {
-        ...fs,
-        utimes: (path, atime, mtime, callback) => setTimeout(() => callback(new Error('foo')), 6000),
-    };
+    const customFs = { ...fs };
 
     const deferred = pDefer();
 
@@ -438,6 +433,8 @@ it('should call the compromised function if lock was acquired by someone else du
         onCompromised: handleCompromised,
     });
 
+    customFs.utimes = (path, atime, mtime, callback) => setTimeout(() => callback(new Error('foo')), 6000);
+
     await pDelay(5500);
 
     await lockfile.lock(`${tmpDir}/foo`, { stale: 5000 });

test/unlock.test.js

@@ -114,7 +114,7 @@ it('should stop updating the lockfile mtime', async () => {
     // First update occurs at 2000ms
     await pDelay(2500);
 
-    expect(customFs.utimes).toHaveBeenCalledTimes(0);
+    expect(customFs.utimes).toHaveBeenCalledTimes(1);
 }, 10000);
 
 it('should stop updating the lockfile mtime (slow fs)', async () => {
@@ -133,7 +133,7 @@ it('should stop updating the lockfile mtime (slow fs)', async () => {
 
     await pDelay(3000);
 
-    expect(customFs.utimes).toHaveBeenCalledTimes(1);
+    expect(customFs.utimes).toHaveBeenCalledTimes(2);
 }, 10000);
 
 it('should stop updating the lockfile mtime (slow fs + new lock)', async () => {
@@ -154,7 +154,7 @@ it('should stop updating the lockfile mtime (slow fs + new lock)', async () => {
 
     await pDelay(3000);
 
-    expect(customFs.utimes).toHaveBeenCalledTimes(1);
+    expect(customFs.utimes).toHaveBeenCalledTimes(2);
 }, 10000);
 
 it('should resolve symlinks by default', async () => {