add mongo transaction lock when getting or updating projects

This commit is contained in:
Hayden Faulds 2018-02-01 15:31:42 +00:00
parent fc1e94b14a
commit cbc6fb232d
10 changed files with 115 additions and 76 deletions

View file

@ -6,15 +6,24 @@ settings = require('settings-sharelatex')
CooldownManager = require '../Cooldown/CooldownManager'
Errors = require '../Errors/Errors'
Folder = require('../../models/Folder').Folder
LockManager = require('../../infrastructure/LockManager')
Project = require('../../models/Project').Project
ProjectEntityHandler = require('./ProjectEntityHandler')
ProjectGetter = require('./ProjectGetter')
ProjectLocator = require('./ProjectLocator')
SafePath = require './SafePath'
wrapWithLock = (methodWithoutLock) ->
methodWithLock = (project_id, args..., callback) ->
LockManager.mongoTransactionLock.runWithLock project_id,
(cb) -> methodWithoutLock project_id, args..., cb
callback
methodWithLock.withoutLock = methodWithoutLock
methodWithLock
module.exports = ProjectEntityMongoUpdateHandler = self =
addDoc: (project_id, folder_id, doc, callback = (err, result) ->) ->
ProjectGetter.getProject project_id, {rootFolder:true, name:true}, (err, project) ->
addDoc: wrapWithLock (project_id, folder_id, doc, callback = (err, result) ->) ->
ProjectGetter.getProjectWithoutLock project_id, {rootFolder:true, name:true}, (err, project) ->
if err?
logger.err project_id:project_id, err:err, "error getting project for add doc"
return callback(err)
@ -22,8 +31,8 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
self._confirmFolder project, folder_id, (folder_id) =>
self._putElement project, folder_id, doc, "doc", callback
addFile: (project_id, folder_id, fileRef, callback = (error, result, project) ->)->
ProjectGetter.getProject project_id, {rootFolder:true, name:true}, (err, project) ->
addFile: wrapWithLock (project_id, folder_id, fileRef, callback = (error, result, project) ->)->
ProjectGetter.getProjectWithoutLock project_id, {rootFolder:true, name:true}, (err, project) ->
if err?
logger.err project_id:project_id, err:err, "error getting project for add file"
return callback(err)
@ -31,8 +40,8 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
self._confirmFolder project, folder_id, (folder_id)->
self._putElement project, folder_id, fileRef, "file", callback
replaceFile: (project_id, file_id, callback) ->
ProjectGetter.getProject project_id, {rootFolder: true, name:true}, (err, project) ->
replaceFile: wrapWithLock (project_id, file_id, callback) ->
ProjectGetter.getProjectWithoutLock project_id, {rootFolder: true, name:true}, (err, project) ->
return callback(err) if err?
ProjectLocator.findElement {project:project, element_id: file_id, type: 'file'}, (err, fileRef, path)=>
return callback(err) if err?
@ -48,7 +57,7 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
return callback(err) if err?
callback null, fileRef, project, path
mkdirp: (project_id, path, callback) ->
mkdirp: wrapWithLock (project_id, path, callback) ->
folders = path.split('/')
folders = _.select folders, (folder)->
return folder.length != 0
@ -69,7 +78,7 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
ProjectLocator.findElementByPath project, builtUpPath, (err, foundFolder)=>
if !foundFolder?
logger.log path:path, project_id:project._id, folderName:folderName, "making folder from mkdirp"
self.addFolder project_id, parentFolder_id, folderName, (err, newFolder, parentFolder_id)->
self.addFolder.withoutLock project_id, parentFolder_id, folderName, (err, newFolder, parentFolder_id)->
return callback(err) if err?
newFolder.parentFolder_id = parentFolder_id
previousFolders.push newFolder
@ -86,8 +95,8 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
!folder.filterOut
callback null, folders, lastFolder
moveEntity: (project_id, entity_id, destFolderId, entityType, callback = (error) ->) ->
ProjectGetter.getProject project_id, {rootFolder:true, name:true}, (err, project) ->
moveEntity: wrapWithLock (project_id, entity_id, destFolderId, entityType, callback = (error) ->) ->
ProjectGetter.getProjectWithoutLock project_id, {rootFolder:true, name:true}, (err, project) ->
return callback(err) if err?
ProjectLocator.findElement {project, element_id: entity_id, type: entityType}, (err, entity, entityPath)->
return callback(err) if err?
@ -106,8 +115,8 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
changes = {oldDocs, newDocs, oldFiles, newFiles}
callback null, project.name, startPath, endPath, entity.rev, changes, callback
deleteEntity: (project_id, entity_id, entityType, callback) ->
ProjectGetter.getProject project_id, {name:true, rootFolder:true}, (error, project) ->
deleteEntity: wrapWithLock (project_id, entity_id, entityType, callback) ->
ProjectGetter.getProjectWithoutLock project_id, {name:true, rootFolder:true}, (error, project) ->
return callback(error) if error?
ProjectLocator.findElement {project: project, element_id: entity_id, type: entityType}, (error, entity, path) ->
return callback(error) if error?
@ -115,8 +124,8 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
return callback(error) if error?
callback null, entity, path, project
renameEntity: (project_id, entity_id, entityType, newName, callback) ->
ProjectGetter.getProject project_id, {rootFolder:true, name:true}, (error, project)=>
renameEntity: wrapWithLock (project_id, entity_id, entityType, newName, callback) ->
ProjectGetter.getProjectWithoutLock project_id, {rootFolder:true, name:true}, (error, project)=>
return callback(error) if error?
ProjectEntityHandler.getAllEntitiesFromProject project, (error, oldDocs, oldFiles) =>
return callback(error) if error?
@ -138,8 +147,8 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
changes = {oldDocs, newDocs, oldFiles, newFiles}
callback null, project.name, startPath, endPath, entity.rev, changes, callback
addFolder: (project_id, parentFolder_id, folderName, callback) ->
ProjectGetter.getProject project_id, {rootFolder:true, name:true}, (err, project) ->
addFolder: wrapWithLock (project_id, parentFolder_id, folderName, callback) ->
ProjectGetter.getProjectWithoutLock project_id, {rootFolder:true, name:true}, (err, project) ->
if err?
logger.err project_id:project_id, err:err, "error getting project for add folder"
return callback(err)

View file

@ -20,7 +20,7 @@ TpdsUpdateSender = require('../ThirdPartyDataStore/TpdsUpdateSender')
wrapWithLock = (methodWithoutLock) ->
methodWithLock = (project_id, args..., callback) ->
LockManager.runWithLock project_id,
LockManager.sequentialProjectStructureUpdateLock.runWithLock project_id,
(cb) -> methodWithoutLock project_id, args..., cb
callback
methodWithLock.withoutLock = methodWithoutLock

View file

@ -5,6 +5,7 @@ ObjectId = mongojs.ObjectId
async = require "async"
Project = require("../../models/Project").Project
logger = require("logger-sharelatex")
LockManager = require("../../infrastructure/LockManager")
module.exports = ProjectGetter =
EXCLUDE_DEPTH: 8
@ -26,7 +27,15 @@ module.exports = ProjectGetter =
callback error, projects[0]
getProject: (query, projection, callback = (error, project) ->) ->
getProject: (project_id, projection, callback = (error, project) ->) ->
if projection?.rootFolder
LockManager.mongoTransactionLock.runWithLock project_id,
(cb) -> ProjectGetter.getProjectWithoutLock project_id, projection, cb
callback
else
ProjectGetter.getProjectWithoutLock project_id, projection, callback
getProjectWithoutLock: (query, projection, callback = (error, project) ->) ->
if !query?
return callback("no query provided")
@ -44,7 +53,7 @@ module.exports = ProjectGetter =
logger.log query:query, err:err, type:typeof(query), "malformed get request"
return callback(err)
db.projects.find query, projection, (err, project)->
db.projects.find query, projection, (err, project) ->
if err?
logger.err err:err, query:query, projection:projection, "error getting project"
return callback(err)

View file

@ -9,10 +9,36 @@ module.exports = LockManager =
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.
_blockingKey : (key)-> "lock:web:{#{key}}"
# This lock is used whenever we read or write to an existing project's
# structure. Some operations to project structure cannot be done atomically
# in mongo, this lock is used to prevent reading the structure between two
# parts of a staged update.
#
# This lock should only be called by ProjectEntityHandler and ProjectGetter
mongoTransactionLock:
runWithLock: (project_id, runner = ( (releaseLock = (error) ->) -> ), callback = ( (error) -> )) ->
LockManager._runWithLock "lock:web:mongoTransaction:{#{project_id}}", runner, callback
tryLock : (key, callback = (err, isFree)->)->
rclient.set LockManager._blockingKey(key), "locked", "EX", LockManager.REDIS_LOCK_EXPIRY, "NX", (err, gotLock)->
# This lock is used to make sure that the project structure updates are made
# sequentially. In particular the updates must be made in mongo and sent to
# the doc-updater in the same order.
#
# This lock is generally called at a high level.
sequentialProjectStructureUpdateLock:
runWithLock: (project_id, runner = ( (releaseLock = (error) ->) -> ), callback = ( (error) -> )) ->
LockManager._runWithLock "lock:web:sequentialProjectStructureUpdateLock:{#{project_id}}", runner, callback
_runWithLock: (key, runner = ( (releaseLock = (error) ->) -> ), callback = ( (error) -> )) ->
LockManager._getLock key, (error) ->
return callback(error) if error?
runner (error1, values...) ->
LockManager._releaseLock key, (error2) ->
error = error1 or error2
return callback(error) if error?
callback null, values...
_tryLock : (key, callback = (err, isFree)->)->
rclient.set key, "locked", "EX", LockManager.REDIS_LOCK_EXPIRY, "NX", (err, gotLock)->
return callback(err) if err?
if gotLock == "OK"
metrics.inc "lock-not-blocking"
@ -22,22 +48,22 @@ module.exports = LockManager =
logger.log key: key, redis_response: gotLock, "lock is locked"
callback err, false
getLock: (key, callback = (error) ->) ->
_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) ->
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)->)->
_checkLock: (key, callback = (err, isFree)->)->
multi = rclient.multi()
multi.exists LockManager._blockingKey(key)
multi.exists key
multi.exec (err, replys)->
return callback(err) if err?
exists = parseInt replys[0]
@ -48,14 +74,5 @@ module.exports = LockManager =
metrics.inc "lock-not-blocking"
callback err, true
releaseLock: (key, callback)->
rclient.del LockManager._blockingKey(key), callback
runWithLock: (key, runner = ( (releaseLock = (error) ->) -> ), callback = ( (error) -> )) ->
LockManager.getLock key, (error) ->
return callback(error) if error?
runner (error1, values...) ->
LockManager.releaseLock key, (error2) ->
error = error1 or error2
return callback(error) if error?
callback null, values...
_releaseLock: (key, callback)->
rclient.del key, callback

View file

@ -35,11 +35,15 @@ describe 'ProjectEntityMongoUpdateHandler', ->
}
"../Cooldown/CooldownManager": @CooldownManager = {}
'../../models/Folder': Folder:@FolderModel
"../../infrastructure/LockManager":@LockManager =
mongoTransactionLock:
runWithLock:
sinon.spy((key, runner, callback) -> runner(callback))
'../../models/Project': Project:@ProjectModel = {}
'./ProjectEntityHandler': @ProjectEntityHandler = {}
'./ProjectLocator': @ProjectLocator = {}
"./ProjectGetter": @ProjectGetter =
getProject: sinon.stub().yields(null, @project)
getProjectWithoutLock: sinon.stub().yields(null, @project)
afterEach ->
tk.reset()
@ -53,7 +57,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
@subject.addDoc project_id, folder_id, @doc, @callback
it 'gets the project', ->
@ProjectGetter.getProject
@ProjectGetter.getProjectWithoutLock
.calledWith(project_id, {rootFolder:true, name: true})
.should.equal true
@ -76,7 +80,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
@subject.addFile project_id, folder_id, @file, @callback
it 'gets the project', ->
@ProjectGetter.getProject
@ProjectGetter.getProjectWithoutLock
.calledWith(project_id, {rootFolder:true, name: true})
.should.equal true
@ -100,7 +104,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
@subject.replaceFile project_id, file_id, @callback
it 'gets the project', ->
@ProjectGetter.getProject
@ProjectGetter.getProjectWithoutLock
.calledWith(project_id, {rootFolder:true, name: true})
.should.equal true
@ -141,8 +145,9 @@ describe 'ProjectEntityMongoUpdateHandler', ->
cb "level1 is not the last foler "
else
cb null, @parentFolder
@subject.addFolder = (project_id, parentFolder_id, folderName, callback) =>
callback null, {name:folderName}, @parentFolder_id
@subject.addFolder =
withoutLock: (project_id, parentFolder_id, folderName, callback) =>
callback null, {name:folderName}, @parentFolder_id
it 'should return the root folder if the path is just a slash', (done)->
path = "/"
@ -217,7 +222,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
@subject.moveEntity project_id, doc_id, folder_id, "docs", @callback
it 'should get the project', ->
@ProjectGetter.getProject
@ProjectGetter.getProjectWithoutLock
.calledWith(project_id, {rootFolder:true, name:true})
.should.equal true
@ -256,7 +261,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
@subject.deleteEntity project_id, doc_id, 'doc', @callback
it "should get the project", ->
@ProjectGetter.getProject
@ProjectGetter.getProjectWithoutLock
.calledWith(project_id, {name:true, rootFolder:true})
.should.equal true
@ -284,7 +289,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
@doc = _id: doc_id, name: "old.tex", rev: 1
@folder = _id: folder_id
@ProjectGetter.getProject = sinon.stub().yields(null, @project)
@ProjectGetter.getProjectWithoutLock = sinon.stub().yields(null, @project)
@ProjectEntityHandler.getAllEntitiesFromProject = sinon.stub()
@ProjectEntityHandler.getAllEntitiesFromProject
@ -301,7 +306,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
@subject.renameEntity project_id, doc_id, 'doc', @newName, @callback
it 'should get the project', ->
@ProjectGetter.getProject
@ProjectGetter.getProjectWithoutLock
.calledWith(project_id, {rootFolder:true, name:true})
.should.equal true
@ -339,7 +344,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
@subject.addFolder project_id, folder_id, @folderName, @callback
it 'gets the project', ->
@ProjectGetter.getProject
@ProjectGetter.getProjectWithoutLock
.calledWith(project_id, {rootFolder:true, name: true})
.should.equal true

View file

@ -52,8 +52,9 @@ describe 'ProjectEntityUpdateHandler', ->
'../../models/File': File:@FileModel
'../FileStore/FileStoreHandler':@FileStoreHandler
"../../infrastructure/LockManager":@LockManager =
runWithLock:
sinon.spy((key, runner, callback) -> runner(callback))
sequentialProjectStructureUpdateLock:
runWithLock:
sinon.spy((key, runner, callback) -> runner(callback))
'../../models/Project': Project:@ProjectModel = {}
"./ProjectGetter": @ProjectGetter = {}
'./ProjectLocator': @ProjectLocator = {}

View file

@ -2,9 +2,7 @@ sinon = require('sinon')
assert = require('assert')
path = require('path')
modulePath = path.join __dirname, '../../../../../app/js/infrastructure/LockManager.js'
project_id = 1234
doc_id = 5678
blockingKey = "lock:web:{#{doc_id}}"
lockKey = "lock:web:{#{5678}}"
SandboxedModule = require('sandboxed-module')
describe 'LockManager - checking the lock', ()->
@ -29,20 +27,20 @@ describe 'LockManager - checking the lock', ()->
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
LockManager._checkLock lockKey, (err, docIsLocked)->
existsStub.calledWith(lockKey).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")
LockManager.checkLock doc_id, (err, free)->
LockManager._checkLock lockKey, (err, free)->
free.should.equal true
done()
it 'should return false if the key does exists', (done)->
execStub.callsArgWith(0, null, "1")
LockManager.checkLock doc_id, (err, free)->
LockManager._checkLock lockKey, (err, free)->
free.should.equal false
done()

View file

@ -2,8 +2,7 @@ sinon = require('sinon')
assert = require('assert')
path = require('path')
modulePath = path.join __dirname, '../../../../../app/js/infrastructure/LockManager.js'
project_id = 1234
doc_id = 5678
lockKey = "lock:web:{#{5678}}"
SandboxedModule = require('sandboxed-module')
describe 'LockManager - releasing the lock', ()->
@ -20,7 +19,7 @@ describe 'LockManager - releasing the lock', ()->
LockManager = SandboxedModule.require(modulePath, requires: mocks)
it 'should put a all data into memory', (done)->
LockManager.releaseLock doc_id, ->
deleteStub.calledWith("lock:web:{#{doc_id}}").should.equal true
LockManager._releaseLock lockKey, ->
deleteStub.calledWith(lockKey).should.equal true
done()

View file

@ -20,18 +20,18 @@ describe 'LockManager - getting the lock', ->
describe "when the lock is not set", ->
beforeEach (done) ->
@LockManager.tryLock = sinon.stub().callsArgWith(1, null, true)
@LockManager.getLock @doc_id, (args...) =>
@LockManager._tryLock = sinon.stub().callsArgWith(1, null, true)
@LockManager._getLock @doc_id, (args...) =>
@callback(args...)
done()
it "should try to get the lock", ->
@LockManager.tryLock
@LockManager._tryLock
.calledWith(@doc_id)
.should.equal true
it "should only need to try once", ->
@LockManager.tryLock.callCount.should.equal 1
@LockManager._tryLock.callCount.should.equal 1
it "should return the callback", ->
@callback.calledWith(null).should.equal true
@ -41,20 +41,20 @@ describe 'LockManager - getting the lock', ->
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
sinon.spy @LockManager, "tryLock"
sinon.spy @LockManager, "_tryLock"
@LockManager.getLock @doc_id, (args...) =>
@LockManager._getLock @doc_id, (args...) =>
@callback(args...)
done()
it "should call tryLock multiple times until free", ->
(@LockManager.tryLock.callCount > 1).should.equal true
(@LockManager._tryLock.callCount > 1).should.equal true
it "should return the callback", ->
@callback.calledWith(null).should.equal true
@ -63,8 +63,8 @@ describe 'LockManager - getting the lock', ->
beforeEach (done) ->
time = Date.now()
@LockManager.MAX_LOCK_WAIT_TIME = 5
@LockManager.tryLock = sinon.stub().callsArgWith(1, null, false)
@LockManager.getLock @doc_id, (args...) =>
@LockManager._tryLock = sinon.stub().callsArgWith(1, null, false)
@LockManager._getLock @doc_id, (args...) =>
@callback(args...)
done()

View file

@ -14,17 +14,18 @@ describe 'LockManager - trying the lock', ->
auth:->
set: @set = sinon.stub()
"settings-sharelatex":{redis:{}}
"metrics-sharelatex": inc:->
"metrics-sharelatex": inc:->
@callback = sinon.stub()
@doc_id = "doc-id-123"
@key = "lock:web:{#{@doc_id}}"
describe "when the lock is not set", ->
beforeEach ->
@set.callsArgWith(5, null, "OK")
@LockManager.tryLock @doc_id, @callback
@LockManager._tryLock @key, @callback
it "should set the lock key with an expiry if it is not set", ->
@set.calledWith("lock:web:{#{@doc_id}}", "locked", "EX", 30, "NX")
@set.calledWith(@key, "locked", "EX", 30, "NX")
.should.equal true
it "should return the callback with true", ->
@ -33,7 +34,7 @@ describe 'LockManager - trying the lock', ->
describe "when the lock is already set", ->
beforeEach ->
@set.callsArgWith(5, null, null)
@LockManager.tryLock @doc_id, @callback
@LockManager._tryLock @key, @callback
it "should return the callback with false", ->
@callback.calledWith(null, false).should.equal true