Use signed locks so only the locking party can remove their lock

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
 
 	

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

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

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

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
 		
 
 

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

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

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