From 6aa6b221621754f66319a92745d98c6304ad3498 Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Mon, 9 Jul 2018 20:03:49 -0400 Subject: [PATCH] Implement proper locking to avoid parallel writes conflicts Fixes: #107 Fixes: #89 Fixes: #40 Signed-off-by: Juan Cruz Viotti --- .travis.yml | 3 ++ lib/storage.js | 75 ++++++++++++++++++++++++++++++++++--------- lib/utils.js | 16 +++++++++ package.json | 6 ++-- stress/process.js | 62 +++++++++++++++++++++++++++++++++++ stress/start.sh | 8 +++++ tests/storage.spec.js | 14 +++++--- 7 files changed, 160 insertions(+), 24 deletions(-) create mode 100644 stress/process.js create mode 100755 stress/start.sh diff --git a/.travis.yml b/.travis.yml index c7fd09a..2bec17a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,5 +3,8 @@ node_js: - "4.0" before_script: - export DISPLAY=:99.0; sh -e /etc/init.d/xvfb start +script: + - npm test + - ./stress/start.sh notifications: email: false diff --git a/lib/storage.js b/lib/storage.js index c2d46d5..ff08aa4 100644 --- a/lib/storage.js +++ b/lib/storage.js @@ -34,9 +34,19 @@ const fs = require('fs'); const rimraf = require('rimraf'); const mkdirp = require('mkdirp'); const path = require('path'); -const RWlock = require('rwlock'); const utils = require('./utils'); -const lock = new RWlock(); +const lockFile = require('lockfile'); + +/** + * @summary Lock options + * @type {Object} + * @private + */ +const lockOptions = { + stale: 10000, + retries: Infinity, + retryWait: 50 +}; /** * @summary Get the default data path @@ -122,12 +132,26 @@ exports.get = function(key, options, callback) { options = options || {}; callback = callback || _.noop; + var fileName = null; async.waterfall([ async.asyncify(_.partial(utils.getFileName, key, { dataPath: options.dataPath })), - function(fileName, callback) { + function(result, callback) { + fileName = result; + mkdirp(path.dirname(fileName), callback); + }, + function(made, next) { + lockFile.lock(utils.getLockFileName(fileName), lockOptions, function(error) { + if (error && error.code === 'EEXIST') { + return exports.get(key, options, callback); + } + + return next(error); + }); + }, + function(callback) { fs.readFile(fileName, { encoding: 'utf8' }, function(error, object) { @@ -151,7 +175,15 @@ exports.get = function(key, options, callback) { } return callback(null, objectJSON); } - ], callback); + ], function(error, result) { + lockFile.unlock(utils.getLockFileName(fileName), function(lockError) { + if (error) { + return callback(error); + } + + return callback(lockError, result); + }); + }); }; /** @@ -267,12 +299,14 @@ exports.set = function(key, json, options, callback) { options = options || {}; callback = callback || _.noop; + var fileName = null; async.waterfall([ async.asyncify(_.partial(utils.getFileName, key, { dataPath: options.dataPath })), - function(fileName, callback) { + function(result, callback) { + fileName = result; const data = JSON.stringify(json); if (!data) { @@ -281,21 +315,30 @@ exports.set = function(key, json, options, callback) { // Create the directory in case it doesn't exist yet mkdirp(path.dirname(fileName), function(error) { - if (error) { - return callback(error); + return callback(error, data); + }); + }, + function(data, next) { + lockFile.lock(utils.getLockFileName(fileName), lockOptions, function(error) { + if (error && error.code === 'EEXIST') { + return exports.set(key, json, options, callback); } - // Ensure parallel writes don't corrupt the data - lock.writeLock(function(releaseLock) { - fs.writeFile(fileName, data, function(error) { - releaseLock(); - return callback(error); - }); - }); + return next(error, fileName, data); }); - + }, + function(fileName, data, callback) { + fs.writeFile(fileName, data, callback); } - ], callback); + ], function(error) { + lockFile.unlock(utils.getLockFileName(fileName), function(lockError) { + if (error) { + return callback(error); + } + + return callback(lockError); + }); + }); }; /** diff --git a/lib/utils.js b/lib/utils.js index 8f18c4a..c481d9b 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -125,3 +125,19 @@ exports.getFileName = function(key, options) { return path.join(options.dataPath || exports.getDataPath(), escapedFileName); }; + +/** + * @summary Get the lock file out of a file name + * @function + * @public + * + * @param {String} fileName - file name + * @returns {String} lock file name + * + * @example + * let lockFileName = utils.getLockFileName('foo'); + * console.log(lockFileName); + */ +exports.getLockFileName = function(fileName) { + return fileName + '.lock'; +}; diff --git a/package.json b/package.json index 8295629..06e7e5d 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ }, "scripts": { "test": "npm run lint && electron-mocha --recursive tests -R spec && electron-mocha --renderer --recursive tests -R spec", - "lint": "jshint --config .jshintrc --reporter unix lib tests", + "lint": "jshint --config .jshintrc --reporter unix lib tests stress", "readme": "jsdoc2md --template doc/README.hbs lib/storage.js > README.md" }, "keywords": [ @@ -36,9 +36,9 @@ "dependencies": { "async": "^2.0.0", "electron": "^2.0.4", + "lockfile": "^1.0.4", "lodash": "^4.0.1", "mkdirp": "^0.5.1", - "rimraf": "^2.5.1", - "rwlock": "^5.0.0" + "rimraf": "^2.5.1" } } diff --git a/stress/process.js b/stress/process.js new file mode 100644 index 0000000..2c8c185 --- /dev/null +++ b/stress/process.js @@ -0,0 +1,62 @@ +/* + * The MIT License + * + * Copyright (c) 2016 Juan Cruz Viotti. https://github.com/jviotti + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +'use strict'; + +const async = require('async'); +const storage = require('..'); +const value = process.argv[2]; + +const retry = function(callback, times) { + if (times === 0) { + return callback(); + } + + async.waterfall([ + function(next) { + storage.set('foo', { + value: value + }, next); + }, + function(next) { + storage.get('foo', next); + } + ], function(error, result) { + if (error) { + return callback(error); + } + + console.log(process.pid, times, result); + retry(callback, times - 1); + }); +}; + +retry(function(error) { + if (error) { + console.error(error); + process.exit(1); + } else { + process.exit(0); + } +}, 2000); diff --git a/stress/start.sh b/stress/start.sh new file mode 100755 index 0000000..5c832ff --- /dev/null +++ b/stress/start.sh @@ -0,0 +1,8 @@ +#!/bin/sh + +set -e + +./node_modules/.bin/electron stress/process.js xxx & PID1=$! +./node_modules/.bin/electron stress/process.js xxxxxx & PID2=$! +wait $PID1 +wait $PID2 diff --git a/tests/storage.spec.js b/tests/storage.spec.js index 6d53905..e091bb8 100644 --- a/tests/storage.spec.js +++ b/tests/storage.spec.js @@ -40,7 +40,7 @@ const app = electron.app || electron.remote.app; describe('Electron JSON Storage', function() { - this.timeout(20000); + this.timeout(100000); // Ensure each test case is always ran in a clean state beforeEach(function(done) { @@ -232,18 +232,22 @@ describe('Electron JSON Storage', function() { it('should return nothing given the wrong data path', function(done) { if (os.platform() === 'win32') { - storage.setDataPath('C:\\electron-json-storage'); + storage.setDataPath('C:\\tmp\\electron-json-storage'); } else { - storage.setDataPath('/electron-json-storage'); + storage.setDataPath('/tmp/electron-json-storage'); } - storage.get('foo', function(error, data) { + async.waterfall([ + storage.clear, + function(callback) { + storage.get('foo', callback); + } + ], function(error, data) { m.chai.expect(error).to.not.exist; m.chai.expect(data).to.deep.equal({}); done(); }); }); - }); describe('given stored keys with a colon', function() {