Add a lock around processing updates

services/track-changes/app/coffee/HistoryManager.coffee

@@ -1,6 +1,7 @@
 MongoManager = require "./MongoManager"
 RedisManager = require "./RedisManager"
 UpdateCompressor = require "./UpdateCompressor"
+LockManager = require "./LockManager"
 logger = require "logger-sharelatex"
 
 module.exports = HistoryManager =
@@ -37,5 +38,10 @@ module.exports = HistoryManager =
 					return callback(error) if error?
 					callback()
 
-	processUncompressUpdatesWithLock: (doc_id, callback = (error) ->) ->
+	processUncompressedUpdatesWithLock: (doc_id, callback = (error) ->) ->
+		LockManager.runWithLock(
+			"HistoryLock:#{doc_id}",
+			HistoryManager.processUncompressedUpdates,
+			callback
+		)
 

services/track-changes/app/coffee/LockManager.coffee

@@ -0,0 +1,54 @@
+Settings = require "settings-sharelatex"
+redis = require('redis')
+redisConf = Settings.redis?.web or {host: "localhost", port: 6379}
+rclient = redis.createClient(redisConf.port, redisConf.host)
+rclient.auth(redisConf.password)
+
+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: 10 # seconds
+
+	tryLock : (key, callback = (err, gotLock) ->) ->
+		rclient.set key, "locked", "EX", @LOCK_TTL, "NX", (err, gotLock)->
+			return callback(err) if err?
+			if gotLock == "OK"
+				callback err, true
+			else
+				callback err, false
+
+	getLock: (key, callback = (error) ->) ->
+		startTime = Date.now()
+		do attempt = () ->
+			if Date.now() - startTime > LockManager.MAX_LOCK_WAIT_TIME
+				return callback(new Error("Timeout"))
+
+			LockManager.tryLock key, (error, gotLock) ->
+				return callback(error) if error?
+				if gotLock
+					callback(null)
+				else
+					setTimeout attempt, LockManager.LOCK_TEST_INTERVAL
+
+	checkLock: (key, callback = (err, isFree) ->) ->
+		rclient.exists key, (err, exists) ->
+			return callback(err) if err?
+			exists = parseInt exists
+			if exists == 1
+				callback err, false
+			else
+				callback err, true
+
+	releaseLock: (key, callback) ->
+		rclient.del key, callback
+
+	runWithLock: (key, runner = ( (releaseLock = (error) ->) -> ), callback = ( (error) -> )) ->
+		LockManager.getLock key, (error) ->
+			return callback(error) if error?
+			runner (error1) ->
+				LockManager.releaseLock key, (error2) ->
+					error = error1 or error2
+					return callback(error) if error?
+					callback()
+
+	

services/track-changes/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee

@@ -11,6 +11,7 @@ describe "HistoryManager", ->
 			"./UpdateCompressor": @UpdateCompressor = {}
 			"./MongoManager" : @MongoManager = {}
 			"./RedisManager" : @RedisManager = {}
+			"./LockManager"  : @LockManager = {}
 			"logger-sharelatex": { log: sinon.stub() }
 		@doc_id = "doc-id-123"
 		@callback = sinon.stub()
@@ -136,4 +137,19 @@ describe "HistoryManager", ->
 		it "should call the callback", ->
 			@callback.called.should.equal true
 
+	describe "processCompressedUpdatesWithLock", ->
+		beforeEach ->
+			@HistoryManager.processUncompressedUpdates = sinon.stub()
+			@LockManager.runWithLock = sinon.stub().callsArg(2)
+			@HistoryManager.processUncompressedUpdatesWithLock @doc_id, @callback
+
+		it "should run processUncompressedUpdates with the lock", ->
+			@LockManager.runWithLock
+				.calledWith(
+					"HistoryLock:#{@doc_id}",
+					@HistoryManager.processUncompressedUpdates
+				)
+				.should.equal true
 
+		it "should call the callback", ->
+			@callback.called.should.equal true

services/track-changes/test/unit/coffee/LockManager/LockManagerTests.coffee

@@ -0,0 +1,171 @@
+sinon = require('sinon')
+chai = require('chai')
+should = chai.should()
+expect = chai.expect
+modulePath = "../../../../app/js/LockManager.js"
+SandboxedModule = require('sandboxed-module')
+
+describe "LockManager", ->
+	beforeEach ->
+		@LockManager = SandboxedModule.require modulePath, requires:
+			"redis":
+				createClient: () => @rclient =
+					auth: sinon.stub()
+			"settings-sharelatex": @Settings = {}
+		@key = "lock-key"
+		@callback = sinon.stub()
+
+	describe "checkLock", ->
+		describe "when the lock is taken", ->
+			beforeEach ->
+				@rclient.exists = sinon.stub().callsArgWith(1, null, "1")
+				@LockManager.checkLock @key, @callback
+
+			it "should check the lock in redis", ->
+				@rclient.exists
+					.calledWith(@key)
+					.should.equal true
+
+			it "should return the callback with false", ->
+				@callback.calledWith(null, false).should.equal true
+
+		describe "when the lock is free", ->
+			beforeEach ->
+				@rclient.exists = sinon.stub().callsArgWith(1, null, "0")
+				@LockManager.checkLock @key, @callback
+
+			it "should return the callback with true", ->
+				@callback.calledWith(null, true).should.equal true
+
+
+	describe "tryLock", ->
+		describe "when the lock is taken", ->
+			beforeEach ->
+				@rclient.set = sinon.stub().callsArgWith(5, null, null)
+				@LockManager.tryLock @key, @callback
+
+			it "should check the lock in redis", ->
+				@rclient.set
+					.calledWith(@key, "locked", "EX", @LockManager.LOCK_TTL, "NX")
+					.should.equal true
+
+			it "should return the callback with false", ->
+				@callback.calledWith(null, false).should.equal true
+
+		describe "when the lock is free", ->
+			beforeEach ->
+				@rclient.set = sinon.stub().callsArgWith(5, null, "OK")
+				@LockManager.tryLock @key, @callback
+
+			it "should return the callback with true", ->
+				@callback.calledWith(null, true).should.equal true
+
+	describe "deleteLock", ->
+		beforeEach -> 
+			beforeEach ->
+				@rclient.del = sinon.stub().callsArg(1)
+				@LockManager.deleteLock @key, @callback
+
+			it "should delete the lock in redis", ->
+				@rclient.del
+					.calledWith(key)
+					.should.equal true
+
+			it "should call the callback", ->
+				@callback.called.should.equal true
+
+	describe "getLock", ->
+		describe "when the lock is not taken", ->
+			beforeEach (done) ->
+				@LockManager.tryLock = sinon.stub().callsArgWith(1, null, true)
+				@LockManager.getLock @key, (args...) =>
+					@callback(args...)
+					done()
+
+			it "should try to get the lock", ->
+				@LockManager.tryLock
+					.calledWith(@key)
+					.should.equal true
+
+			it "should only need to try once", ->
+				@LockManager.tryLock.callCount.should.equal 1
+
+			it "should return the callback", ->
+				@callback.calledWith(null).should.equal true
+
+		describe "when the lock is initially set", ->
+			beforeEach (done) ->
+				startTime = Date.now()
+				@LockManager.LOCK_TEST_INTERVAL = 5
+				@LockManager.tryLock = (doc_id, callback = (error, isFree) ->) ->
+					if Date.now() - startTime < 20
+						callback null, false
+					else
+						callback null, true
+				sinon.spy @LockManager, "tryLock"
+
+				@LockManager.getLock @key, (args...) =>
+					@callback(args...)
+					done()
+
+			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
+
+		describe "when the lock times out", ->
+			beforeEach (done) ->
+				time = Date.now()
+				@LockManager.MAX_LOCK_WAIT_TIME = 5
+				@LockManager.tryLock = sinon.stub().callsArgWith(1, null, false)
+				@LockManager.getLock @key, (args...) =>
+					@callback(args...)
+					done()
+
+			it "should return the callback with an error", ->
+				@callback.calledWith(new Error("timeout")).should.equal true
+
+	describe "runWithLock", ->
+		describe "with successful run", ->
+			beforeEach ->
+				@runner = (releaseLock = (error) ->) ->
+					releaseLock()
+				sinon.spy @, "runner"
+				@LockManager.getLock = sinon.stub().callsArg(1)
+				@LockManager.releaseLock = sinon.stub().callsArg(1)
+				@LockManager.runWithLock @key, @runner, @callback
+
+			it "should get the lock", ->
+				@LockManager.getLock
+					.calledWith(@key)
+					.should.equal true
+
+			it "should run the passed function", ->
+				@runner.called.should.equal true
+
+			it "should release the lock", ->
+				@LockManager.releaseLock
+					.calledWith(@key)
+					.should.equal true
+
+			it "should call the callback", ->
+				@callback.called.should.equal true
+
+		describe "when the runner function returns an error", ->
+			beforeEach ->
+				@error = new Error("oops")
+				@runner = (releaseLock = (error) ->) =>
+					releaseLock(@error)
+				sinon.spy @, "runner"
+				@LockManager.getLock = sinon.stub().callsArg(1)
+				@LockManager.releaseLock = sinon.stub().callsArg(1)
+				@LockManager.runWithLock @key, @runner, @callback
+
+			it "should release the lock", ->
+				@LockManager.releaseLock
+					.calledWith(@key)
+					.should.equal true
+
+			it "should call the callback with the error", ->
+				@callback.calledWith(@error).should.equal true