diff --git a/services/document-updater/app.js b/services/document-updater/app.js index 69dc579385..c489abf0de 100644 --- a/services/document-updater/app.js +++ b/services/document-updater/app.js @@ -149,6 +149,7 @@ app.delete('/project', HttpController.deleteMultipleProjects) app.post('/project/:project_id', HttpController.updateProject) app.post( '/project/:project_id/history/resync', + longerTimeout, HttpController.resyncProjectHistory ) app.post('/project/:project_id/flush', HttpController.flushProject) @@ -258,3 +259,8 @@ for (const signal of [ ]) { process.on(signal, shutdownCleanly(signal)) } + +function longerTimeout(req, res, next) { + res.setTimeout(6 * 60 * 1000) + next() +} diff --git a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js index b4777d140c..ffd1805726 100644 --- a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js +++ b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js @@ -221,6 +221,7 @@ function resyncProjectHistory( path: `/project/${projectId}/history/resync`, json: { docs, files, projectHistoryId }, method: 'POST', + timeout: 6 * 60 * 1000, // allow 6 minutes for resync }, projectId, 'resync-project-history', @@ -299,6 +300,7 @@ function _makeRequest(options, projectId, metricsKey, callback) { url: `${settings.apis.documentupdater.url}${options.path}`, json: options.json, method: options.method || 'GET', + timeout: options.timeout || 30 * 1000, }, function (error, res, body) { timer.done() diff --git a/services/web/app/src/Features/History/HistoryController.js b/services/web/app/src/Features/History/HistoryController.js index 13af24f352..7fe4223bfe 100644 --- a/services/web/app/src/Features/History/HistoryController.js +++ b/services/web/app/src/Features/History/HistoryController.js @@ -97,6 +97,8 @@ module.exports = HistoryController = { }, resyncProjectHistory(req, res, next) { + // increase timeout to 6 minutes + res.setTimeout(6 * 60 * 1000) const projectId = req.params.Project_id ProjectEntityUpdateHandler.resyncProjectHistory(projectId, function (err) { if (err instanceof Errors.ProjectHistoryDisabledError) { diff --git a/services/web/app/src/Features/History/HistoryManager.js b/services/web/app/src/Features/History/HistoryManager.js index bf566f32e8..51831f16d6 100644 --- a/services/web/app/src/Features/History/HistoryManager.js +++ b/services/web/app/src/Features/History/HistoryManager.js @@ -99,6 +99,7 @@ async function resyncProject(projectId, options = {}) { await request.post({ url: `${settings.apis.project_history.url}/project/${projectId}/resync`, json: body, + timeout: 6 * 60 * 1000, }) } catch (err) { throw OError.tag(err, 'failed to resync project history', { projectId }) diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js index 5bb7ec5d37..9f2889b3db 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js @@ -30,7 +30,7 @@ const VALID_ROOT_DOC_REGEXP = new RegExp( 'i' ) -function wrapWithLock(methodWithoutLock) { +function wrapWithLock(methodWithoutLock, lockManager = LockManager) { // 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. @@ -39,7 +39,7 @@ function wrapWithLock(methodWithoutLock) { const adjustedLength = Math.max(rest.length, 1) const args = rest.slice(0, adjustedLength - 1) const callback = rest[adjustedLength - 1] - LockManager.runWithLock( + lockManager.runWithLock( LOCK_NAMESPACE, projectId, cb => methodWithoutLock(projectId, ...args, cb), @@ -56,7 +56,7 @@ function wrapWithLock(methodWithoutLock) { const adjustedLength = Math.max(rest.length, 1) const args = rest.slice(0, adjustedLength - 1) const callback = rest[adjustedLength - 1] - LockManager.runWithLock( + lockManager.runWithLock( LOCK_NAMESPACE, projectId, cb => mainTask(projectId, ...args, cb), @@ -1331,62 +1331,64 @@ const ProjectEntityUpdateHandler = { // This doesn't directly update project structure but we need to take the lock // to prevent anything else being queued before the resync update - resyncProjectHistory: wrapWithLock((projectId, callback) => - ProjectGetter.getProject( - projectId, - { rootFolder: true, overleaf: true }, - (error, project) => { - if (error != null) { - return callback(error) - } - - const projectHistoryId = - project && - project.overleaf && - project.overleaf.history && - project.overleaf.history.id - if (projectHistoryId == null) { - error = new Errors.ProjectHistoryDisabledError( - `project history not enabled for ${projectId}` - ) - return callback(error) - } - - let { docs, files, folders } = - ProjectEntityHandler.getAllEntitiesFromProject(project) - // _checkFileTree() must be passed the folders before docs and - // files - ProjectEntityUpdateHandler._checkFiletree( - projectId, - projectHistoryId, - [...folders, ...docs, ...files], - error => { - if (error) { - return callback(error) - } - docs = _.map(docs, doc => ({ - doc: doc.doc._id, - path: doc.path, - })) - - files = _.map(files, file => ({ - file: file.file._id, - path: file.path, - url: FileStoreHandler._buildUrl(projectId, file.file._id), - _hash: file.file.hash, - })) - - DocumentUpdaterHandler.resyncProjectHistory( - projectId, - projectHistoryId, - docs, - files, - callback - ) + resyncProjectHistory: wrapWithLock( + (projectId, callback) => + ProjectGetter.getProject( + projectId, + { rootFolder: true, overleaf: true }, + (error, project) => { + if (error != null) { + return callback(error) } - ) - } - ) + + const projectHistoryId = + project && + project.overleaf && + project.overleaf.history && + project.overleaf.history.id + if (projectHistoryId == null) { + error = new Errors.ProjectHistoryDisabledError( + `project history not enabled for ${projectId}` + ) + return callback(error) + } + + let { docs, files, folders } = + ProjectEntityHandler.getAllEntitiesFromProject(project) + // _checkFileTree() must be passed the folders before docs and + // files + ProjectEntityUpdateHandler._checkFiletree( + projectId, + projectHistoryId, + [...folders, ...docs, ...files], + error => { + if (error) { + return callback(error) + } + docs = _.map(docs, doc => ({ + doc: doc.doc._id, + path: doc.path, + })) + + files = _.map(files, file => ({ + file: file.file._id, + path: file.path, + url: FileStoreHandler._buildUrl(projectId, file.file._id), + _hash: file.file.hash, + })) + + DocumentUpdaterHandler.resyncProjectHistory( + projectId, + projectHistoryId, + docs, + files, + callback + ) + } + ) + } + ), + LockManager.withTimeout(6 * 60) // use an extended lock for the resync operations ), _checkFiletree(projectId, projectHistoryId, entities, callback) { diff --git a/services/web/app/src/infrastructure/LockManager.js b/services/web/app/src/infrastructure/LockManager.js index dbf9a01868..3bd7208ea8 100644 --- a/services/web/app/src/infrastructure/LockManager.js +++ b/services/web/app/src/infrastructure/LockManager.js @@ -5,13 +5,34 @@ const { callbackify, promisify } = require('util') const RedisWebLocker = require('@overleaf/redis-wrapper/RedisWebLocker') -const LockManager = new RedisWebLocker({ - rclient, - getKey(namespace, id) { - return `lock:web:${namespace}:${id}` - }, - options: settings.lockManager, -}) +// this method creates a lock manager with the provided timeout options +function createLockManager(options) { + return new RedisWebLocker({ + rclient, + getKey(namespace, id) { + return `lock:web:${namespace}:${id}` + }, + options, + }) +} + +// this is the default lock manager for web +const LockManager = createLockManager(settings.lockManager) + +// this method creates a lock manager with a configurable timeout +// it shares the lock keys with the default lock manager +LockManager.withTimeout = function (timeout) { + const overrides = { + redisLockExpiry: timeout, // in seconds + slowExecutionThreshold: 0.5 * timeout * 1000, // in ms + } + const lockManagerSettingsWithTimeout = Object.assign( + {}, + settings.lockManager, + overrides + ) + return createLockManager(lockManagerSettingsWithTimeout) +} // need to bind the promisified function when it is part of a class // see https://nodejs.org/dist/latest-v16.x/docs/api/util.html#utilpromisifyoriginal diff --git a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js index 63873717ca..7e90a52ee3 100644 --- a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js +++ b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js @@ -350,6 +350,7 @@ describe('DocumentUpdaterHandler', function () { user_id: this.user_id, }, method: 'POST', + timeout: 30 * 1000, }) .should.equal(true) }) @@ -439,6 +440,7 @@ describe('DocumentUpdaterHandler', function () { url: `${this.settings.apis.documentupdater.url}/project/${this.project_id}/doc/${this.doc_id}?fromVersion=${this.fromVersion}`, method: 'GET', json: true, + timeout: 30 * 1000, }) .should.equal(true) }) @@ -658,6 +660,7 @@ describe('DocumentUpdaterHandler', function () { change_ids: [this.change_id], }, method: 'POST', + timeout: 30 * 1000, }) .should.equal(true) }) @@ -881,6 +884,7 @@ describe('DocumentUpdaterHandler', function () { version: this.version, projectHistoryId: this.projectHistoryId, }, + timeout: 30 * 1000, }) .should.equal(true) done() @@ -926,6 +930,7 @@ describe('DocumentUpdaterHandler', function () { version: this.version, projectHistoryId: this.projectHistoryId, }, + timeout: 30 * 1000, }) .should.equal(true) done() @@ -975,6 +980,7 @@ describe('DocumentUpdaterHandler', function () { version: this.version, projectHistoryId: this.projectHistoryId, }, + timeout: 30 * 1000, }) .should.equal(true) done() @@ -1018,6 +1024,7 @@ describe('DocumentUpdaterHandler', function () { version: this.version, projectHistoryId: this.projectHistoryId, }, + timeout: 30 * 1000, }) .should.equal(true) done() @@ -1080,6 +1087,7 @@ describe('DocumentUpdaterHandler', function () { version: this.version, projectHistoryId: this.projectHistoryId, }, + timeout: 30 * 1000, }) done() } diff --git a/services/web/test/unit/src/History/HistoryControllerTests.js b/services/web/test/unit/src/History/HistoryControllerTests.js index 65308617e9..0b3a95fe61 100644 --- a/services/web/test/unit/src/History/HistoryControllerTests.js +++ b/services/web/test/unit/src/History/HistoryControllerTests.js @@ -311,7 +311,7 @@ describe('HistoryController', function () { beforeEach(function () { this.project_id = 'mock-project-id' this.req = { params: { Project_id: this.project_id } } - this.res = { sendStatus: sinon.stub() } + this.res = { setTimeout: sinon.stub(), sendStatus: sinon.stub() } this.next = sinon.stub() this.error = new Errors.ProjectHistoryDisabledError() @@ -335,7 +335,7 @@ describe('HistoryController', function () { beforeEach(function () { this.project_id = 'mock-project-id' this.req = { params: { Project_id: this.project_id } } - this.res = { sendStatus: sinon.stub() } + this.res = { setTimeout: sinon.stub(), sendStatus: sinon.stub() } this.next = sinon.stub() this.ProjectEntityUpdateHandler.resyncProjectHistory = sinon @@ -349,6 +349,10 @@ describe('HistoryController', function () { ) }) + it('sets an extended response timeout', function () { + this.res.setTimeout.should.have.been.calledWith(6 * 60 * 1000) + }) + it('resyncs the project', function () { return this.ProjectEntityUpdateHandler.resyncProjectHistory .calledWith(this.project_id) diff --git a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js index ee2175e523..8f5ae8d8fc 100644 --- a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js @@ -88,6 +88,7 @@ describe('ProjectEntityUpdateHandler', function () { runWithLock: sinon.spy((namespace, id, runner, callback) => runner(callback) ), + withTimeout: sinon.stub().returns(this.LockManager), } this.ProjectModel = { updateOne: sinon.stub(), @@ -1954,6 +1955,10 @@ describe('ProjectEntityUpdateHandler', function () { .should.equal(true) }) + it('uses an extended timeout', function () { + this.LockManager.withTimeout.calledWith(6 * 60).should.equal(true) + }) + it('tells the doc updater to sync the project', function () { const docs = [ {