From 0ff4be1e2fba02ac1106fc7803c288ffb4211460 Mon Sep 17 00:00:00 2001 From: pimlie Date: Sat, 30 Mar 2019 13:29:23 +0100 Subject: [PATCH 1/3] feat: add strict mode currently used for toggling compromised lock behaviour --- README.md | 1 + lib/lockfile.js | 32 +++++++++++++++++++------ test/lock.test.js | 2 +- test/release.test.js | 57 ++++++++++++++++++++++++++++++++++++++++++++ test/util/wait.js | 23 ++++++++++++++++++ 5 files changed, 107 insertions(+), 8 deletions(-) create mode 100644 test/util/wait.js diff --git a/README.md b/README.md index 5d2b1e0..f10a29e 100644 --- a/README.md +++ b/README.md @@ -74,6 +74,7 @@ If the lock succeeds, a `release` function is provided that should be called whe Available options: - `stale`: Duration in milliseconds in which the lock is considered stale, defaults to `10000` (minimum value is `5000`) +- `strict`: Toggle strict mode. In non-strict mode a compromised lockfile will still be removed, also in strict mode calling release on a compromised lock will throw an error. Defaults to `false` - `update`: The interval in milliseconds in which the lockfile's `mtime` will be updated, defaults to `stale/2` (minimum value is `1000`, maximum value is `stale/2`) - `retries`: The number of retries or a [retry](https://www.npmjs.org/package/retry) options object, defaults to `0` - `realpath`: Resolve symlinks using realpath, defaults to `true` (note that if `true`, the `file` must exist previously) diff --git a/lib/lockfile.js b/lib/lockfile.js index 2078e82..df910a1 100644 --- a/lib/lockfile.js +++ b/lib/lockfile.js @@ -128,8 +128,8 @@ function updateLock(file, options) { options.fs.utimes(getLockFile(file, options), mtime, mtime, (err) => { const isOverThreshold = lock.lastUpdate + options.stale < Date.now(); - // Ignore if the lock was released - if (lock.released) { + // Ignore if the lock was compromised or released + if (lock.compromised || lock.released) { return; } @@ -162,13 +162,14 @@ function updateLock(file, options) { // may be using this module outside of NodeJS (e.g., in an electron app), // and in those cases `setTimeout` return an integer. /* istanbul ignore else */ - if (lock.updateTimeout.unref) { + if (lock.updateTimeout && lock.updateTimeout.unref) { lock.updateTimeout.unref(); } } function setLockAsCompromised(file, lock, err) { - // Signal the lock has been released + // Signal the lock has been compromised + lock.compromised = true; lock.released = true; // Cancel lock mtime update @@ -178,11 +179,23 @@ function setLockAsCompromised(file, lock, err) { clearTimeout(lock.updateTimeout); } - if (locks[file] === lock) { - delete locks[file]; + if (lock.options.strict) { + if (locks[file] === lock) { + delete locks[file]; + } + + lock.options.onCompromised(err); + + return; } - lock.options.onCompromised(err); + removeLock(file, lock.options, () => { + if (locks[file] === lock) { + delete locks[file]; + } + + lock.options.onCompromised(err); + }); } // ---------------------------------------------------------- @@ -191,6 +204,7 @@ function lock(file, options, callback) { /* istanbul ignore next */ options = { stale: 10000, + strict: false, update: null, realpath: true, retries: 0, @@ -236,6 +250,10 @@ function lock(file, options, callback) { updateLock(file, options); callback(null, (releasedCallback) => { + if (!options.strict && lock.compromised) { + return releasedCallback && releasedCallback(); + } + if (lock.released) { return releasedCallback && releasedCallback(Object.assign(new Error('Lock is already released'), { code: 'ERELEASED' })); diff --git a/test/lock.test.js b/test/lock.test.js index 4846874..ad974b6 100644 --- a/test/lock.test.js +++ b/test/lock.test.js @@ -426,7 +426,7 @@ it('should call the compromised function if lock was acquired by someone else du const handleCompromised = (err) => { expect(err.code).toBe('ECOMPROMISED'); - expect(fs.existsSync(`${tmpDir}/foo.lock`)).toBe(true); + expect(fs.existsSync(`${tmpDir}/foo.lock`)).toBe(false); deferred.resolve(); }; diff --git a/test/release.test.js b/test/release.test.js index 3150b31..f12e247 100644 --- a/test/release.test.js +++ b/test/release.test.js @@ -5,6 +5,7 @@ const mkdirp = require('mkdirp'); const rimraf = require('rimraf'); const lockfile = require('../'); const unlockAll = require('./util/unlockAll'); +const { waitUntil } = require('./util/wait'); const tmpDir = `${__dirname}/tmp`; @@ -52,3 +53,59 @@ it('should fail when releasing twice', async () => { expect(err.code).toBe('ERELEASED'); } }); + +it('shouldnt error on releasing compromised lock in non-strict mode', async () => { + const lockPath = `${tmpDir}/foo`; + const lockDir = `${lockPath}.lock`; + const stale = 10; + const onCompromised = jest.fn(); + + jest.useFakeTimers(); + fs.writeFileSync(lockPath, ''); + + const release = await lockfile.lock(lockPath, { stale, onCompromised }); + const mtime = (new Date().getTime() / 1000) - stale; + + fs.utimesSync(lockDir, mtime, mtime); + + // Advance timers to trigger the updateTimeout so the lock gets compromised + jest.advanceTimersByTime(stale * 2 * 1000); + + // switch back to use realTimers to wait max 5 seconds as there is no + // other way to wait for all callbacks from the internal timeout to return + jest.useRealTimers(); + await waitUntil(() => onCompromised.mock.calls.length, 5, 50); + + expect(onCompromised).toHaveBeenCalledTimes(1); + + await expect(release()).resolves.not.toThrow(); + expect(fs.existsSync(lockDir)).toBe(false); +}); + +it('should error on releasing compromised lock in strict mode', async () => { + const lockPath = `${tmpDir}/foo`; + const lockDir = `${lockPath}.lock`; + const stale = 10; + const onCompromised = jest.fn(); + + jest.useFakeTimers(); + fs.writeFileSync(lockPath, ''); + + const release = await lockfile.lock(lockPath, { strict: true, stale, onCompromised }); + const mtime = (new Date().getTime() / 1000) - stale; + + fs.utimesSync(lockDir, mtime, mtime); + + // Advance timers to trigger the updateTimeout so the lock gets compromised + jest.advanceTimersByTime(stale * 2 * 1000); + + // switch back to use realTimers to wait max 5 seconds as there is no + // other way to wait for all callbacks from the internal timeout to return + jest.useRealTimers(); + await waitUntil(() => onCompromised.mock.calls.length, 5, 50); + + expect(onCompromised).toHaveBeenCalledTimes(1); + + await expect(release()).rejects.toThrow('Lock is already released'); + expect(fs.existsSync(lockDir)).toBe(true); +}); diff --git a/test/util/wait.js b/test/util/wait.js new file mode 100644 index 0000000..9232749 --- /dev/null +++ b/test/util/wait.js @@ -0,0 +1,23 @@ +'use strict'; + +function waitFor(ms) { + return new Promise((resolve) => setTimeout(resolve, ms || 0)); +} + +async function waitUntil(condition, duration = 20, interval = 250) { + let iterator = -1; + const steps = Math.ceil(duration * 1000 / interval); + + let ready; + + do { + ready = condition(); + await waitFor(interval); // eslint-disable-line no-await-in-loop + iterator += 1; + } while (!ready && iterator < steps); + + return ready; +} + +module.exports.waitFor = waitFor; +module.exports.waitUntil = waitUntil; From 2239bf943fcefcbfef49947aa677d5b0dc5108e7 Mon Sep 17 00:00:00 2001 From: pimlie Date: Wed, 3 Apr 2019 14:38:52 +0200 Subject: [PATCH 2/3] chore: revert strict mode feat: dont trigger error on compromised lock --- README.md | 1 - lib/lockfile.js | 32 ++++++++++---------------------- test/lock.test.js | 2 +- test/release.test.js | 31 ++----------------------------- 4 files changed, 13 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index f10a29e..5d2b1e0 100644 --- a/README.md +++ b/README.md @@ -74,7 +74,6 @@ If the lock succeeds, a `release` function is provided that should be called whe Available options: - `stale`: Duration in milliseconds in which the lock is considered stale, defaults to `10000` (minimum value is `5000`) -- `strict`: Toggle strict mode. In non-strict mode a compromised lockfile will still be removed, also in strict mode calling release on a compromised lock will throw an error. Defaults to `false` - `update`: The interval in milliseconds in which the lockfile's `mtime` will be updated, defaults to `stale/2` (minimum value is `1000`, maximum value is `stale/2`) - `retries`: The number of retries or a [retry](https://www.npmjs.org/package/retry) options object, defaults to `0` - `realpath`: Resolve symlinks using realpath, defaults to `true` (note that if `true`, the `file` must exist previously) diff --git a/lib/lockfile.js b/lib/lockfile.js index df910a1..2f73cf2 100644 --- a/lib/lockfile.js +++ b/lib/lockfile.js @@ -179,23 +179,11 @@ function setLockAsCompromised(file, lock, err) { clearTimeout(lock.updateTimeout); } - if (lock.options.strict) { - if (locks[file] === lock) { - delete locks[file]; - } - - lock.options.onCompromised(err); - - return; + if (locks[file] === lock) { + delete locks[file]; } - removeLock(file, lock.options, () => { - if (locks[file] === lock) { - delete locks[file]; - } - - lock.options.onCompromised(err); - }); + lock.options.onCompromised(err); } // ---------------------------------------------------------- @@ -204,7 +192,6 @@ function lock(file, options, callback) { /* istanbul ignore next */ options = { stale: 10000, - strict: false, update: null, realpath: true, retries: 0, @@ -250,13 +237,14 @@ function lock(file, options, callback) { updateLock(file, options); callback(null, (releasedCallback) => { - if (!options.strict && lock.compromised) { - return releasedCallback && releasedCallback(); - } - if (lock.released) { - return releasedCallback && - releasedCallback(Object.assign(new Error('Lock is already released'), { code: 'ERELEASED' })); + let error; + + if (!lock.compromised) { + error = Object.assign(new Error('Lock is already released'), { code: 'ERELEASED' }); + } + + return releasedCallback && releasedCallback(error); } // Not necessary to use realpath twice when unlocking diff --git a/test/lock.test.js b/test/lock.test.js index ad974b6..4846874 100644 --- a/test/lock.test.js +++ b/test/lock.test.js @@ -426,7 +426,7 @@ it('should call the compromised function if lock was acquired by someone else du const handleCompromised = (err) => { expect(err.code).toBe('ECOMPROMISED'); - expect(fs.existsSync(`${tmpDir}/foo.lock`)).toBe(false); + expect(fs.existsSync(`${tmpDir}/foo.lock`)).toBe(true); deferred.resolve(); }; diff --git a/test/release.test.js b/test/release.test.js index f12e247..b769a92 100644 --- a/test/release.test.js +++ b/test/release.test.js @@ -54,7 +54,7 @@ it('should fail when releasing twice', async () => { } }); -it('shouldnt error on releasing compromised lock in non-strict mode', async () => { +it('shouldnt error on releasing compromised lock', async () => { const lockPath = `${tmpDir}/foo`; const lockDir = `${lockPath}.lock`; const stale = 10; @@ -79,33 +79,6 @@ it('shouldnt error on releasing compromised lock in non-strict mode', async () = expect(onCompromised).toHaveBeenCalledTimes(1); await expect(release()).resolves.not.toThrow(); - expect(fs.existsSync(lockDir)).toBe(false); -}); - -it('should error on releasing compromised lock in strict mode', async () => { - const lockPath = `${tmpDir}/foo`; - const lockDir = `${lockPath}.lock`; - const stale = 10; - const onCompromised = jest.fn(); - - jest.useFakeTimers(); - fs.writeFileSync(lockPath, ''); - - const release = await lockfile.lock(lockPath, { strict: true, stale, onCompromised }); - const mtime = (new Date().getTime() / 1000) - stale; - - fs.utimesSync(lockDir, mtime, mtime); - - // Advance timers to trigger the updateTimeout so the lock gets compromised - jest.advanceTimersByTime(stale * 2 * 1000); - - // switch back to use realTimers to wait max 5 seconds as there is no - // other way to wait for all callbacks from the internal timeout to return - jest.useRealTimers(); - await waitUntil(() => onCompromised.mock.calls.length, 5, 50); - - expect(onCompromised).toHaveBeenCalledTimes(1); - - await expect(release()).rejects.toThrow('Lock is already released'); expect(fs.existsSync(lockDir)).toBe(true); }); + From c19d934a327b5835a4feb8733e7c0bc0c0a5542a Mon Sep 17 00:00:00 2001 From: pimlie Date: Wed, 3 Apr 2019 14:39:55 +0200 Subject: [PATCH 3/3] chore: remove unnecessary if branch --- lib/lockfile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/lockfile.js b/lib/lockfile.js index 2f73cf2..ff6d183 100644 --- a/lib/lockfile.js +++ b/lib/lockfile.js @@ -129,7 +129,7 @@ function updateLock(file, options) { const isOverThreshold = lock.lastUpdate + options.stale < Date.now(); // Ignore if the lock was compromised or released - if (lock.compromised || lock.released) { + if (lock.released) { return; }