From 945c728db2a2a540b9eeeed6f43cd158d63804f3 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 13 Apr 2016 11:59:56 +0100 Subject: [PATCH] Use signed locks so only the locking party can remove their lock --- .../app/coffee/LockManager.coffee | 45 +++++++++++++------ .../app/coffee/UpdateManager.coffee | 18 ++++---- .../coffee/LockManager/CheckingTheLock.coffee | 23 ++-------- .../LockManager/ReleasingTheLock.coffee | 9 ++-- .../coffee/LockManager/getLockTests.coffee | 20 +++++---- .../coffee/LockManager/tryLockTests.coffee | 8 ++-- .../UpdateManager/ApplyingUpdates.coffee | 8 ++-- .../lockUpdatesAndDoTests.coffee | 11 ++--- 8 files changed, 77 insertions(+), 65 deletions(-) diff --git a/services/document-updater/app/coffee/LockManager.coffee b/services/document-updater/app/coffee/LockManager.coffee index 0facb8519b..7901d3860a 100644 --- a/services/document-updater/app/coffee/LockManager.coffee +++ b/services/document-updater/app/coffee/LockManager.coffee @@ -4,42 +4,60 @@ redis = require("redis-sharelatex") rclient = redis.createClient(Settings.redis.web) keys = require('./RedisKeyBuilder') logger = require "logger-sharelatex" +os = require "os" +crypto = require "crypto" + +HOST = os.hostname() +PID = process.pid 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 - REDIS_LOCK_EXPIRY: 30 # seconds. Time until lock auto expires in redis. + LOCK_TTL: 30 # seconds. Time until lock auto expires in redis. + + # 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() + RND = crypto.randomBytes(4).toString('hex') + return "locked:host=#{HOST}:pid=#{PID}:random=#{RND}:time=#{time}" + + unlockScript: 'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end'; tryLock : (doc_id, callback = (err, isFree)->)-> - rclient.set keys.blockingKey(doc_id: doc_id), "locked", "EX", LockManager.REDIS_LOCK_EXPIRY, "NX", (err, gotLock)-> + lockValue = LockManager.randomLock() + key = keys.blockingKey(doc_id:doc_id) + rclient.set key, lockValue, "EX", @LOCK_TTL, "NX", (err, gotLock)-> return callback(err) if err? if gotLock == "OK" metrics.inc "doc-not-blocking" - callback err, true + callback err, true, lockValue else metrics.inc "doc-blocking" - logger.log doc_id: doc_id, redis_response: gotLock, "doc is locked" + logger.log {doc_id, lockValue}, "doc is locked" callback err, false getLock: (doc_id, callback = (error) ->) -> startTime = Date.now() do attempt = () -> if Date.now() - startTime > LockManager.MAX_LOCK_WAIT_TIME - return callback(new Error("Timeout")) + e = new Error("Timeout") + e.doc_id = doc_id + return callback(e) - LockManager.tryLock doc_id, (error, gotLock) -> + LockManager.tryLock doc_id, (error, gotLock, lockValue) -> return callback(error) if error? if gotLock - callback(null) + callback(null, lockValue) else setTimeout attempt, LockManager.LOCK_TEST_INTERVAL checkLock: (doc_id, callback = (err, isFree)->)-> - multi = rclient.multi() - multi.exists keys.blockingKey(doc_id:doc_id) - multi.exec (err, replys)-> + key = keys.blockingKey(doc_id:doc_id) + rclient.exists key, (err, exists) -> return callback(err) if err? - exists = parseInt replys[0] + exists = parseInt exists if exists == 1 metrics.inc "doc-blocking" callback err, false @@ -47,7 +65,8 @@ module.exports = LockManager = metrics.inc "doc-not-blocking" callback err, true - releaseLock: (doc_id, callback)-> - rclient.del keys.blockingKey(doc_id:doc_id), callback + releaseLock: (doc_id, lockValue, callback)-> + key = keys.blockingKey(doc_id:doc_id) + rclient.eval LockManager.unlockScript, 1, key, lockValue, callback diff --git a/services/document-updater/app/coffee/UpdateManager.coffee b/services/document-updater/app/coffee/UpdateManager.coffee index 8765c330ff..2ab74feda1 100644 --- a/services/document-updater/app/coffee/UpdateManager.coffee +++ b/services/document-updater/app/coffee/UpdateManager.coffee @@ -29,12 +29,12 @@ module.exports = UpdateManager = callback() processOutstandingUpdatesWithLock: (project_id, doc_id, callback = (error) ->) -> - LockManager.tryLock doc_id, (error, gotLock) => + LockManager.tryLock doc_id, (error, gotLock, lockValue) => return callback(error) if error? return callback() if !gotLock UpdateManager.processOutstandingUpdates project_id, doc_id, (error) -> - return UpdateManager._handleErrorInsideLock(doc_id, error, callback) if error? - LockManager.releaseLock doc_id, (error) => + return UpdateManager._handleErrorInsideLock(doc_id, lockValue, error, callback) if error? + LockManager.releaseLock doc_id, lockValue, (error) => return callback(error) if error? UpdateManager.continueProcessingUpdatesWithLock project_id, doc_id, callback @@ -62,20 +62,20 @@ module.exports = UpdateManager = RedisManager.setDocument doc_id, updatedDocLines, version, callback lockUpdatesAndDo: (method, project_id, doc_id, args..., callback) -> - LockManager.getLock doc_id, (error) -> + LockManager.getLock doc_id, (error, lockValue) -> return callback(error) if error? UpdateManager.processOutstandingUpdates project_id, doc_id, (error) -> - return UpdateManager._handleErrorInsideLock(doc_id, error, callback) if error? + return UpdateManager._handleErrorInsideLock(doc_id, lockValue, error, callback) if error? method project_id, doc_id, args..., (error, response_args...) -> - return UpdateManager._handleErrorInsideLock(doc_id, error, callback) if error? - LockManager.releaseLock doc_id, (error) -> + return UpdateManager._handleErrorInsideLock(doc_id, lockValue, error, callback) if error? + LockManager.releaseLock doc_id, lockValue, (error) -> return callback(error) if error? callback null, response_args... # We held the lock for a while so updates might have queued up UpdateManager.continueProcessingUpdatesWithLock project_id, doc_id - _handleErrorInsideLock: (doc_id, original_error, callback = (error) ->) -> - LockManager.releaseLock doc_id, (lock_error) -> + _handleErrorInsideLock: (doc_id, lockValue, original_error, callback = (error) ->) -> + LockManager.releaseLock doc_id, lockValue, (lock_error) -> callback(original_error) _sanitizeUpdate: (update) -> diff --git a/services/document-updater/test/unit/coffee/LockManager/CheckingTheLock.coffee b/services/document-updater/test/unit/coffee/LockManager/CheckingTheLock.coffee index 84c34f0725..e4514750c8 100644 --- a/services/document-updater/test/unit/coffee/LockManager/CheckingTheLock.coffee +++ b/services/document-updater/test/unit/coffee/LockManager/CheckingTheLock.coffee @@ -9,12 +9,9 @@ doc_id = 5678 blockingKey = "Blocking:#{doc_id}" SandboxedModule = require('sandboxed-module') -describe 'Lock Manager - checking the lock', ()-> +describe 'LockManager - checking the lock', ()-> existsStub = sinon.stub() - setStub = sinon.stub() - exireStub = sinon.stub() - execStub = sinon.stub() mocks = "logger-sharelatex": log:-> @@ -22,30 +19,18 @@ describe 'Lock Manager - checking the lock', ()-> "redis-sharelatex": createClient : ()-> auth:-> - multi: -> - exists: existsStub - expire: exireStub - set: setStub - exec: execStub + exists: existsStub "./Metrics": {inc: () ->} LockManager = SandboxedModule.require(modulePath, requires: mocks) - it 'should check if lock exists but not set or expire', (done)-> - execStub.callsArgWith(0, null, ["1"]) - LockManager.checkLock doc_id, (err, docIsLocked)-> - existsStub.calledWith(blockingKey).should.equal true - setStub.called.should.equal false - exireStub.called.should.equal false - done() - it 'should return true if the key does not exists', (done)-> - execStub.callsArgWith(0, null, "0") + existsStub.yields(null, "0") LockManager.checkLock doc_id, (err, free)-> free.should.equal true done() it 'should return false if the key does exists', (done)-> - execStub.callsArgWith(0, null, "1") + existsStub.yields(null, "1") LockManager.checkLock doc_id, (err, free)-> free.should.equal false done() diff --git a/services/document-updater/test/unit/coffee/LockManager/ReleasingTheLock.coffee b/services/document-updater/test/unit/coffee/LockManager/ReleasingTheLock.coffee index 3b8b09877d..bca2f9124a 100644 --- a/services/document-updater/test/unit/coffee/LockManager/ReleasingTheLock.coffee +++ b/services/document-updater/test/unit/coffee/LockManager/ReleasingTheLock.coffee @@ -10,20 +10,21 @@ SandboxedModule = require('sandboxed-module') describe 'LockManager - releasing the lock', ()-> - deleteStub = sinon.stub().callsArgWith(1) + evalStub = sinon.stub().yields(1) mocks = "logger-sharelatex": log:-> "redis-sharelatex": createClient : ()-> auth:-> - del:deleteStub + eval: evalStub "./Metrics": {inc: () ->} LockManager = SandboxedModule.require(modulePath, requires: mocks) it 'should put a all data into memory', (done)-> - LockManager.releaseLock doc_id, -> - deleteStub.calledWith("Blocking:#{doc_id}").should.equal true + lockValue = "lock-value-stub" + LockManager.releaseLock doc_id, lockValue, -> + evalStub.calledWith(LockManager.unlockScript, 1, "Blocking:#{doc_id}", lockValue).should.equal true done() diff --git a/services/document-updater/test/unit/coffee/LockManager/getLockTests.coffee b/services/document-updater/test/unit/coffee/LockManager/getLockTests.coffee index 026bd81538..84cc3208a3 100644 --- a/services/document-updater/test/unit/coffee/LockManager/getLockTests.coffee +++ b/services/document-updater/test/unit/coffee/LockManager/getLockTests.coffee @@ -17,7 +17,8 @@ describe 'LockManager - getting the lock', -> describe "when the lock is not set", -> beforeEach (done) -> - @LockManager.tryLock = sinon.stub().callsArgWith(1, null, true) + @lockValue = "mock-lock-value" + @LockManager.tryLock = sinon.stub().callsArgWith(1, null, true, @lockValue) @LockManager.getLock @doc_id, (args...) => @callback(args...) done() @@ -30,20 +31,21 @@ describe 'LockManager - getting the lock', -> it "should only need to try once", -> @LockManager.tryLock.callCount.should.equal 1 - it "should return the callback", -> - @callback.calledWith(null).should.equal true + it "should return the callback with the lock value", -> + @callback.calledWith(null, @lockValue).should.equal true describe "when the lock is initially set", -> beforeEach (done) -> + @lockValue = "mock-lock-value" startTime = Date.now() tries = 0 @LockManager.LOCK_TEST_INTERVAL = 5 - @LockManager.tryLock = (doc_id, callback = (error, isFree) ->) -> + @LockManager.tryLock = (doc_id, callback = (error, isFree) ->) => if (Date.now() - startTime < 20) or (tries < 2) tries = tries + 1 callback null, false else - callback null, true + callback null, true, @lockValue sinon.spy @LockManager, "tryLock" @LockManager.getLock @doc_id, (args...) => @@ -53,8 +55,8 @@ describe 'LockManager - getting the lock', -> it "should call tryLock multiple times until free", -> (@LockManager.tryLock.callCount > 1).should.equal true - it "should return the callback", -> - @callback.calledWith(null).should.equal true + it "should return the callback with the lock value", -> + @callback.calledWith(null, @lockValue).should.equal true describe "when the lock times out", -> beforeEach (done) -> @@ -66,7 +68,9 @@ describe 'LockManager - getting the lock', -> done() it "should return the callback with an error", -> - @callback.calledWith(new Error("timeout")).should.equal true + e = new Error("Timeout") + e.doc_id = @doc_id + @callback.calledWith(e).should.equal true diff --git a/services/document-updater/test/unit/coffee/LockManager/tryLockTests.coffee b/services/document-updater/test/unit/coffee/LockManager/tryLockTests.coffee index 0a1ddc00e9..33c3eb3d51 100644 --- a/services/document-updater/test/unit/coffee/LockManager/tryLockTests.coffee +++ b/services/document-updater/test/unit/coffee/LockManager/tryLockTests.coffee @@ -19,15 +19,17 @@ describe 'LockManager - trying the lock', -> describe "when the lock is not set", -> beforeEach -> + @lockValue = "mock-lock-value" + @LockManager.randomLock = sinon.stub().returns @lockValue @set.callsArgWith(5, null, "OK") @LockManager.tryLock @doc_id, @callback it "should set the lock key with an expiry if it is not set", -> - @set.calledWith("Blocking:#{@doc_id}", "locked", "EX", 30, "NX") + @set.calledWith("Blocking:#{@doc_id}", @lockValue, "EX", 30, "NX") .should.equal true - it "should return the callback with true", -> - @callback.calledWith(null, true).should.equal true + it "should return the callback with true and the lock value", -> + @callback.calledWith(null, true, @lockValue).should.equal true describe "when the lock is already set", -> beforeEach -> diff --git a/services/document-updater/test/unit/coffee/UpdateManager/ApplyingUpdates.coffee b/services/document-updater/test/unit/coffee/UpdateManager/ApplyingUpdates.coffee index 70bab70c71..7b41647f7b 100644 --- a/services/document-updater/test/unit/coffee/UpdateManager/ApplyingUpdates.coffee +++ b/services/document-updater/test/unit/coffee/UpdateManager/ApplyingUpdates.coffee @@ -67,8 +67,8 @@ describe "UpdateManager", -> describe "processOutstandingUpdatesWithLock", -> describe "when the lock is free", -> beforeEach -> - @LockManager.tryLock = sinon.stub().callsArgWith(1, null, true) - @LockManager.releaseLock = sinon.stub().callsArg(1) + @LockManager.tryLock = sinon.stub().callsArgWith(1, null, true, @lockValue = "mock-lock-value") + @LockManager.releaseLock = sinon.stub().callsArg(2) @UpdateManager.continueProcessingUpdatesWithLock = sinon.stub().callsArg(2) @UpdateManager.processOutstandingUpdates = sinon.stub().callsArg(2) @@ -80,7 +80,7 @@ describe "UpdateManager", -> @LockManager.tryLock.calledWith(@doc_id).should.equal true it "should free the lock", -> - @LockManager.releaseLock.calledWith(@doc_id).should.equal true + @LockManager.releaseLock.calledWith(@doc_id, @lockValue).should.equal true it "should process the outstanding updates", -> @UpdateManager.processOutstandingUpdates.calledWith(@project_id, @doc_id).should.equal true @@ -101,7 +101,7 @@ describe "UpdateManager", -> @UpdateManager.processOutstandingUpdatesWithLock @project_id, @doc_id, @callback it "should free the lock", -> - @LockManager.releaseLock.calledWith(@doc_id).should.equal true + @LockManager.releaseLock.calledWith(@doc_id, @lockValue).should.equal true it "should return the error in the callback", -> @callback.calledWith(@error).should.equal true diff --git a/services/document-updater/test/unit/coffee/UpdateManager/lockUpdatesAndDoTests.coffee b/services/document-updater/test/unit/coffee/UpdateManager/lockUpdatesAndDoTests.coffee index 74e5a689fa..adba644b27 100644 --- a/services/document-updater/test/unit/coffee/UpdateManager/lockUpdatesAndDoTests.coffee +++ b/services/document-updater/test/unit/coffee/UpdateManager/lockUpdatesAndDoTests.coffee @@ -17,8 +17,9 @@ describe 'UpdateManager - lockUpdatesAndDo', -> @callback = sinon.stub() @arg1 = "argument 1" @response_arg1 = "response argument 1" - @LockManager.getLock = sinon.stub().callsArgWith(1, null, true) - @LockManager.releaseLock = sinon.stub().callsArg(1) + @lockValue = "mock-lock-value" + @LockManager.getLock = sinon.stub().callsArgWith(1, null, @lockValue) + @LockManager.releaseLock = sinon.stub().callsArg(2) describe "successfully", -> beforeEach -> @@ -48,7 +49,7 @@ describe 'UpdateManager - lockUpdatesAndDo', -> it "should release the lock", -> @LockManager.releaseLock - .calledWith(@doc_id) + .calledWith(@doc_id, @lockValue) .should.equal true it "should continue processing updates", -> @@ -62,7 +63,7 @@ describe 'UpdateManager - lockUpdatesAndDo', -> @UpdateManager.lockUpdatesAndDo @method, @project_id, @doc_id, @arg1, @callback it "should free the lock", -> - @LockManager.releaseLock.calledWith(@doc_id).should.equal true + @LockManager.releaseLock.calledWith(@doc_id, @lockValue).should.equal true it "should return the error in the callback", -> @callback.calledWith(@error).should.equal true @@ -74,7 +75,7 @@ describe 'UpdateManager - lockUpdatesAndDo', -> @UpdateManager.lockUpdatesAndDo @method, @project_id, @doc_id, @arg1, @callback it "should free the lock", -> - @LockManager.releaseLock.calledWith(@doc_id).should.equal true + @LockManager.releaseLock.calledWith(@doc_id, @lockValue).should.equal true it "should return the error in the callback", -> @callback.calledWith(@error).should.equal true