Implement proper locking to avoid parallel writes conflicts

Fixes: #107
Fixes: #89
Fixes: #40
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>

.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

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);
+    });
+  });
 };
 
 /**

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';
+};

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"
   }
 }

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);

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

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() {