diff --git a/services/track-changes/app/coffee/LockManager.coffee b/services/track-changes/app/coffee/LockManager.coffee index 48fdc1665a..349410752e 100644 --- a/services/track-changes/app/coffee/LockManager.coffee +++ b/services/track-changes/app/coffee/LockManager.coffee @@ -1,17 +1,34 @@ Settings = require "settings-sharelatex" redis = require("redis-sharelatex") rclient = redis.createClient(Settings.redis.web) +os = require "os" +crypto = require "crypto" + +HOST = os.hostname() +PID = process.pid +RND = crypto.randomBytes(4).toString('hex') +COUNT = 0 module.exports = LockManager = LOCK_TEST_INTERVAL: 50 # 50ms between each test of the lock MAX_LOCK_WAIT_TIME: 10000 # 10s maximum time to spend trying to get the lock LOCK_TTL: 300 # seconds (allow 5 minutes for any operation to complete) + # Use a signed lock value as described in + # http://redis.io/topics/distlock#correct-implementation-with-a-single-instance + # to prevent accidental unlocking by multiple processes + randomLock : () -> + time = Date.now() + return "locked:host=#{HOST}:pid=#{PID}:random=#{RND}:time=#{time}:count=#{COUNT++}" + + unlockScript: 'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end'; + tryLock : (key, callback = (err, gotLock) ->) -> - rclient.set key, "locked", "EX", @LOCK_TTL, "NX", (err, gotLock)-> + lockValue = LockManager.randomLock() + rclient.set key, lockValue, "EX", @LOCK_TTL, "NX", (err, gotLock)-> return callback(err) if err? if gotLock == "OK" - callback err, true + callback err, true, lockValue else callback err, false @@ -21,10 +38,10 @@ module.exports = LockManager = if Date.now() - startTime > LockManager.MAX_LOCK_WAIT_TIME return callback(new Error("Timeout")) - LockManager.tryLock key, (error, gotLock) -> + LockManager.tryLock key, (error, gotLock, lockValue) -> return callback(error) if error? if gotLock - callback(null) + callback(null, lockValue) else setTimeout attempt, LockManager.LOCK_TEST_INTERVAL @@ -37,14 +54,14 @@ module.exports = LockManager = else callback err, true - releaseLock: (key, callback) -> - rclient.del key, callback + releaseLock: (key, lockValue, callback) -> + rclient.eval LockManager.unlockScript, 1, key, lockValue, callback runWithLock: (key, runner = ( (releaseLock = (error) ->) -> ), callback = ( (error) -> )) -> - LockManager.getLock key, (error) -> + LockManager.getLock key, (error, lockValue) -> return callback(error) if error? runner (error1) -> - LockManager.releaseLock key, (error2) -> + LockManager.releaseLock key, lockValue, (error2) -> error = error1 or error2 return callback(error) if error? callback() diff --git a/services/track-changes/test/acceptance/coffee/LockManagerTests.coffee b/services/track-changes/test/acceptance/coffee/LockManagerTests.coffee new file mode 100644 index 0000000000..6de4599a6b --- /dev/null +++ b/services/track-changes/test/acceptance/coffee/LockManagerTests.coffee @@ -0,0 +1,31 @@ +sinon = require "sinon" +chai = require("chai") +chai.should() +expect = chai.expect +mongojs = require "../../../app/js/mongojs" +ObjectId = mongojs.ObjectId +Settings = require "settings-sharelatex" +LockManager = require "../../../app/js/LockManager" +rclient = require("redis").createClient() # Only works locally for now + +describe "Locking document", -> + describe "when the lock has expired in redis", -> + before (done) -> + LockManager.LOCK_TTL = 1 # second + LockManager.runWithLock "doc123", (releaseA) => + # we create a lock A and allow it to expire in redis + setTimeout () -> + # now we create a new lock B and try to release A + LockManager.runWithLock "doc123", (releaseB) => + releaseA() # try to release lock A to see if it wipes out lock B + , (error) -> + # we never release lock B so nothing should happen here + , 1500 # enough time to wait until the lock has expired + , (error) -> + # we get here after trying to release lock A + done() + + it "the new lock should not be removed by the expired locker", (done) -> + LockManager.checkLock "doc123", (err, isFree) -> + expect(isFree).to.equal false + done() diff --git a/services/track-changes/test/unit/coffee/LockManager/LockManagerTests.coffee b/services/track-changes/test/unit/coffee/LockManager/LockManagerTests.coffee index 9ea9c81f15..298665a00b 100644 --- a/services/track-changes/test/unit/coffee/LockManager/LockManagerTests.coffee +++ b/services/track-changes/test/unit/coffee/LockManager/LockManagerTests.coffee @@ -46,11 +46,12 @@ describe "LockManager", -> describe "when the lock is taken", -> beforeEach -> @rclient.set = sinon.stub().callsArgWith(5, null, null) + @LockManager.randomLock = sinon.stub().returns "locked-random-value" @LockManager.tryLock @key, @callback it "should check the lock in redis", -> @rclient.set - .calledWith(@key, "locked", "EX", @LockManager.LOCK_TTL, "NX") + .calledWith(@key, "locked-random-value", "EX", @LockManager.LOCK_TTL, "NX") .should.equal true it "should return the callback with false", -> @@ -137,7 +138,7 @@ describe "LockManager", -> releaseLock() sinon.spy @, "runner" @LockManager.getLock = sinon.stub().callsArg(1) - @LockManager.releaseLock = sinon.stub().callsArg(1) + @LockManager.releaseLock = sinon.stub().callsArg(2) @LockManager.runWithLock @key, @runner, @callback it "should get the lock", -> @@ -163,7 +164,7 @@ describe "LockManager", -> releaseLock(@error) sinon.spy @, "runner" @LockManager.getLock = sinon.stub().callsArg(1) - @LockManager.releaseLock = sinon.stub().callsArg(1) + @LockManager.releaseLock = sinon.stub().callsArg(2) @LockManager.runWithLock @key, @runner, @callback it "should release the lock", ->