From fe62db05a062e45ca5b92be505cbb4de9d24efe2 Mon Sep 17 00:00:00 2001 From: Hayden Faulds Date: Tue, 2 Jan 2018 12:43:41 +0000 Subject: [PATCH 1/2] lock project when uploading a file --- .../Uploads/ProjectUploadController.coffee | 35 +++++++++++-------- .../ProjectUploadControllerTests.coffee | 10 ++++++ 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/services/web/app/coffee/Features/Uploads/ProjectUploadController.coffee b/services/web/app/coffee/Features/Uploads/ProjectUploadController.coffee index ff8c5aa827..523a4ca8a5 100644 --- a/services/web/app/coffee/Features/Uploads/ProjectUploadController.coffee +++ b/services/web/app/coffee/Features/Uploads/ProjectUploadController.coffee @@ -5,6 +5,7 @@ Path = require "path" FileSystemImportManager = require "./FileSystemImportManager" ProjectUploadManager = require "./ProjectUploadManager" AuthenticationController = require('../Authentication/AuthenticationController') +LockManager = require("../../infrastructure/LockManager") module.exports = ProjectUploadController = uploadProject: (req, res, next) -> @@ -37,18 +38,24 @@ module.exports = ProjectUploadController = return res.send success: false logger.log folder_id:folder_id, project_id:project_id, "getting upload file request" user_id = AuthenticationController.getLoggedInUserId(req) - FileSystemImportManager.addEntity user_id, project_id, folder_id, name, path, true, (error, entity) -> - fs.unlink path, -> - timer.done() - if error? - logger.error - err: error, project_id: project_id, file_path: path, - file_name: name, folder_id: folder_id, - "error uploading file" - res.send success: false - else - logger.log - project_id: project_id, file_path: path, file_name: name, folder_id: folder_id - "uploaded file" - res.send success: true, entity_id: entity?._id, entity_type: entity?.type + + LockManager.getLock project_id, (err)-> + if err? + logger.err err:err, project_id:project_id, source:source, "could not get lock to uploadFile" + return callback(err) + FileSystemImportManager.addEntity user_id, project_id, folder_id, name, path, true, (error, entity) -> + LockManager.releaseLock project_id, -> + fs.unlink path, -> + timer.done() + if error? + logger.error + err: error, project_id: project_id, file_path: path, + file_name: name, folder_id: folder_id, + "error uploading file" + res.send success: false + else + logger.log + project_id: project_id, file_path: path, file_name: name, folder_id: folder_id + "uploaded file" + res.send success: true, entity_id: entity?._id, entity_type: entity?.type diff --git a/services/web/test/unit/coffee/Uploads/ProjectUploadControllerTests.coffee b/services/web/test/unit/coffee/Uploads/ProjectUploadControllerTests.coffee index 3e60fc0046..24ff97b30f 100644 --- a/services/web/test/unit/coffee/Uploads/ProjectUploadControllerTests.coffee +++ b/services/web/test/unit/coffee/Uploads/ProjectUploadControllerTests.coffee @@ -17,12 +17,16 @@ describe "ProjectUploadController", -> done: sinon.stub() @AuthenticationController = getLoggedInUserId: sinon.stub().returns(@user_id) + @LockManager = + getLock : sinon.stub().yields() + releaseLock : sinon.stub().yields() @ProjectUploadController = SandboxedModule.require modulePath, requires: "./ProjectUploadManager" : @ProjectUploadManager = {} "./FileSystemImportManager" : @FileSystemImportManager = {} "logger-sharelatex" : @logger = {log: sinon.stub(), error: sinon.stub(), err:->} "metrics-sharelatex": @metrics '../Authentication/AuthenticationController': @AuthenticationController + "../../infrastructure/LockManager": @LockManager "fs" : @fs = {} describe "uploadProject", -> @@ -125,11 +129,17 @@ describe "ProjectUploadController", -> @FileSystemImportManager.addEntity = sinon.stub().callsArgWith(6, null, @entity) @ProjectUploadController.uploadFile @req, @res + it "should take the lock", -> + @LockManager.getLock.calledWith(@project_id).should.equal true + it "should insert the file", -> @FileSystemImportManager.addEntity .calledWith(@user_id, @project_id, @folder_id, @name, @path) .should.equal true + it "should release the lock", -> + @LockManager.releaseLock.calledWith(@project_id).should.equal true + it "should return a successful response to the FileUploader client", -> expect(@res.body).to.deep.equal success: true From 817840840af6938135e2587e707f935d122ae440 Mon Sep 17 00:00:00 2001 From: Hayden Faulds Date: Tue, 9 Jan 2018 16:00:08 +0000 Subject: [PATCH 2/2] use LockManger.runWithLock --- .../Uploads/ProjectUploadController.coffee | 35 +++++++++---------- .../coffee/infrastructure/LockManager.coffee | 9 ++++- .../ProjectUploadControllerTests.coffee | 9 ++--- 3 files changed, 27 insertions(+), 26 deletions(-) diff --git a/services/web/app/coffee/Features/Uploads/ProjectUploadController.coffee b/services/web/app/coffee/Features/Uploads/ProjectUploadController.coffee index 523a4ca8a5..1d81fdb831 100644 --- a/services/web/app/coffee/Features/Uploads/ProjectUploadController.coffee +++ b/services/web/app/coffee/Features/Uploads/ProjectUploadController.coffee @@ -39,23 +39,20 @@ module.exports = ProjectUploadController = logger.log folder_id:folder_id, project_id:project_id, "getting upload file request" user_id = AuthenticationController.getLoggedInUserId(req) - LockManager.getLock project_id, (err)-> - if err? - logger.err err:err, project_id:project_id, source:source, "could not get lock to uploadFile" - return callback(err) - FileSystemImportManager.addEntity user_id, project_id, folder_id, name, path, true, (error, entity) -> - LockManager.releaseLock project_id, -> - fs.unlink path, -> - timer.done() - if error? - logger.error - err: error, project_id: project_id, file_path: path, - file_name: name, folder_id: folder_id, - "error uploading file" - res.send success: false - else - logger.log - project_id: project_id, file_path: path, file_name: name, folder_id: folder_id - "uploaded file" - res.send success: true, entity_id: entity?._id, entity_type: entity?.type + LockManager.runWithLock project_id, + (cb) -> FileSystemImportManager.addEntity user_id, project_id, folder_id, name, path, true, cb + (error, entity) -> + fs.unlink path, -> + timer.done() + if error? + logger.error + err: error, project_id: project_id, file_path: path, + file_name: name, folder_id: folder_id, + "error uploading file" + res.send success: false + else + logger.log + project_id: project_id, file_path: path, file_name: name, folder_id: folder_id + "uploaded file" + res.send success: true, entity_id: entity?._id, entity_type: entity?.type diff --git a/services/web/app/coffee/infrastructure/LockManager.coffee b/services/web/app/coffee/infrastructure/LockManager.coffee index 3064fffabf..5532d5bd94 100644 --- a/services/web/app/coffee/infrastructure/LockManager.coffee +++ b/services/web/app/coffee/infrastructure/LockManager.coffee @@ -51,4 +51,11 @@ module.exports = LockManager = 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... diff --git a/services/web/test/unit/coffee/Uploads/ProjectUploadControllerTests.coffee b/services/web/test/unit/coffee/Uploads/ProjectUploadControllerTests.coffee index 24ff97b30f..488250b647 100644 --- a/services/web/test/unit/coffee/Uploads/ProjectUploadControllerTests.coffee +++ b/services/web/test/unit/coffee/Uploads/ProjectUploadControllerTests.coffee @@ -18,8 +18,8 @@ describe "ProjectUploadController", -> @AuthenticationController = getLoggedInUserId: sinon.stub().returns(@user_id) @LockManager = - getLock : sinon.stub().yields() - releaseLock : sinon.stub().yields() + runWithLock : sinon.spy((key, runner, callback) -> runner(callback)) + @ProjectUploadController = SandboxedModule.require modulePath, requires: "./ProjectUploadManager" : @ProjectUploadManager = {} "./FileSystemImportManager" : @FileSystemImportManager = {} @@ -130,16 +130,13 @@ describe "ProjectUploadController", -> @ProjectUploadController.uploadFile @req, @res it "should take the lock", -> - @LockManager.getLock.calledWith(@project_id).should.equal true + @LockManager.runWithLock.calledWith(@project_id).should.equal true it "should insert the file", -> @FileSystemImportManager.addEntity .calledWith(@user_id, @project_id, @folder_id, @name, @path) .should.equal true - it "should release the lock", -> - @LockManager.releaseLock.calledWith(@project_id).should.equal true - it "should return a successful response to the FileUploader client", -> expect(@res.body).to.deep.equal success: true