diff --git a/services/document-updater/app/js/LockManager.js b/services/document-updater/app/js/LockManager.js deleted file mode 100644 index a65ab9f1e3..0000000000 --- a/services/document-updater/app/js/LockManager.js +++ /dev/null @@ -1,18 +0,0 @@ -const Settings = require('@overleaf/settings') -const redis = require('@overleaf/redis-wrapper') -const rclient = redis.createClient(Settings.redis.lock) -const keys = Settings.redis.lock.key_schema -const RedisLocker = require('@overleaf/redis-wrapper/RedisLocker') - -module.exports = new RedisLocker({ - rclient, - getKey(docId) { - return keys.blockingKey({ doc_id: docId }) - }, - wrapTimeoutError(err, docId) { - err.doc_id = docId - return err - }, - metricsPrefix: 'doc', - lockTTLSeconds: Settings.redisLockTTLSeconds, -}) diff --git a/services/document-updater/app/js/ProjectLockManager.js b/services/document-updater/app/js/ProjectLockManager.js index ce396fd056..733b45de03 100644 --- a/services/document-updater/app/js/ProjectLockManager.js +++ b/services/document-updater/app/js/ProjectLockManager.js @@ -5,8 +5,7 @@ const keys = Settings.redis.lock.key_schema const RedisLocker = require('@overleaf/redis-wrapper/RedisLocker') // Project level lock used to serialise processing of a project's pending -// updates (the `BlockingProject:{project_id}` key). Acquired before the per-doc -// `Blocking:{doc_id}` lock - never the other way around - to avoid deadlocks. +// updates (the `BlockingProject:{project_id}` key). module.exports = new RedisLocker({ rclient, getKey(projectId) { diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index 1eac2d47e2..b793bda437 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/services/document-updater/app/js/UpdateManager.js @@ -1,7 +1,6 @@ // @ts-check const { callbackifyAll } = require('@overleaf/promise-utils') -const LockManager = require('./LockManager') const ProjectLockManager = require('./ProjectLockManager') const RedisManager = require('./RedisManager') const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager') @@ -220,7 +219,7 @@ const UpdateManager = { .map(change => change.metadata.user_id) // Fire-and-forget without awaiting because - // we hold the doc lock here, and the result of the + // we hold the project lock here, and the result of the // notification doesn't affect the update WebApiManager.promises .notifyTrackChangesRejected( @@ -252,7 +251,7 @@ const UpdateManager = { ) // Do this last, since it's a mongo call, and so potentially longest running - // If it overruns the lock, it's ok, since all of our redis work is done + // If it overruns the project lock, it's ok, since all of our redis work is done await SnapshotManager.promises.recordSnapshot( projectId, docId, @@ -277,7 +276,7 @@ const UpdateManager = { /** * Process the project's pending updates, then run the given method on the - * doc, all under the project and doc locks. + * doc, all under the project lock. * * @param {Function} method - called with (projectId, docId, ...args) * @param {string} projectId @@ -291,9 +290,8 @@ const UpdateManager = { doc_id: docId, }) - // Project lock first (always before the per-doc lock, to avoid deadlocks) - // so we can safely drain the shared per-project queue, then the per-doc - // lock for the operation itself. + // Take the project lock so we can safely drain the shared per-project queue + // and then run the operation itself. const projectLockValue = await ProjectLockManager.promises.getLock(projectId) profile.log('getProjectLock') @@ -303,16 +301,11 @@ const UpdateManager = { await UpdateManager.fetchAndApplyProjectUpdates(projectId, profile) profile.log('fetchAndApplyProjectUpdates') - const lockValue = await LockManager.promises.getLock(docId) - profile.log('getLock') + await ProjectLockManager.promises.extendLock(projectId, projectLockValue) + profile.log('extendProjectLock') - try { - result = await method(projectId, docId, ...args) - profile.log('method') - } finally { - await LockManager.promises.releaseLock(docId, lockValue) - profile.log('releaseLock') - } + result = await method(projectId, docId, ...args) + profile.log('method') } finally { await ProjectLockManager.promises.releaseLock(projectId, projectLockValue) profile.log('releaseProjectLock').end() diff --git a/services/document-updater/config/settings.defaults.js b/services/document-updater/config/settings.defaults.js index 6645c72c8a..34836203e5 100755 --- a/services/document-updater/config/settings.defaults.js +++ b/services/document-updater/config/settings.defaults.js @@ -77,9 +77,6 @@ module.exports = { process.env.REDIS_MAX_RETRIES_PER_REQUEST || '20' ), key_schema: { - blockingKey({ doc_id: docId }) { - return `Blocking:{${docId}}` - }, projectBlockingKey({ project_id: projectId }) { return `BlockingProject:{${projectId}}` }, @@ -101,9 +98,6 @@ module.exports = { process.env.REDIS_MAX_RETRIES_PER_REQUEST || '20' ), key_schema: { - blockingKey({ doc_id: docId }) { - return `Blocking:{${docId}}` - }, docLines({ doc_id: docId }) { return `doclines:{${docId}}` }, diff --git a/services/document-updater/scripts/check_redis_mongo_sync_state.js b/services/document-updater/scripts/check_redis_mongo_sync_state.js index 1be0d870b2..998071ecfb 100644 --- a/services/document-updater/scripts/check_redis_mongo_sync_state.js +++ b/services/document-updater/scripts/check_redis_mongo_sync_state.js @@ -4,7 +4,7 @@ const _ = require('lodash') const logger = require('@overleaf/logger') const OError = require('@overleaf/o-error') const Errors = require('../app/js/Errors') -const LockManager = require('../app/js/LockManager') +const ProjectLockManager = require('../app/js/ProjectLockManager') const PersistenceManager = require('../app/js/PersistenceManager') const ProjectFlusher = require('../app/js/ProjectFlusher') const ProjectManager = require('../app/js/ProjectManager') @@ -43,13 +43,16 @@ const COMPARE_AND_SET = class TryAgainError extends Error {} /** + * @param {string} projectId * @param {string} docId * @param {Doc} redisDoc * @param {Doc} mongoDoc * @return {Promise} */ -async function updateDocVersionInRedis(docId, redisDoc, mongoDoc) { - const lockValue = await LockManager.promises.getLock(docId) +async function updateDocVersionInRedis(projectId, docId, redisDoc, mongoDoc) { + // Take the project lock to serialise with document-updater, which now + // processes all of a project's docs under the project lock. + const lockValue = await ProjectLockManager.promises.getLock(projectId) try { const key = Settings.redis.documentupdater.key_schema.docVersion({ doc_id: docId, @@ -68,7 +71,7 @@ async function updateDocVersionInRedis(docId, redisDoc, mongoDoc) { ) } } finally { - await LockManager.promises.releaseLock(docId, lockValue) + await ProjectLockManager.promises.releaseLock(projectId, lockValue) } } @@ -175,7 +178,7 @@ async function processDoc(projectId, docId) { console.log( `Fixing out of sync doc version for doc ${docId} in project ${projectId}: mongo=${mongoDoc.version} > redis=${redisDoc.version}` ) - await updateDocVersionInRedis(docId, redisDoc, mongoDoc) + await updateDocVersionInRedis(projectId, docId, redisDoc, mongoDoc) return false } else { console.error( diff --git a/services/document-updater/test/unit/js/LockManager/CheckingTheLock.js b/services/document-updater/test/unit/js/LockManager/CheckingTheLock.js deleted file mode 100644 index 575ed90702..0000000000 --- a/services/document-updater/test/unit/js/LockManager/CheckingTheLock.js +++ /dev/null @@ -1,65 +0,0 @@ -/* eslint-disable - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS206: Consider reworking classes to avoid initClass - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const sinon = require('sinon') -const assert = require('node:assert') -const path = require('node:path') -const modulePath = path.join(__dirname, '../../../../app/js/LockManager.js') -const projectId = 1234 -const docId = 5678 -const blockingKey = `Blocking:${docId}` -const SandboxedModule = require('sandboxed-module') - -describe('LockManager - checking the lock', function () { - let Profiler - const existsStub = sinon.stub() - - const mocks = { - '@overleaf/redis-wrapper': { - createClient() { - return { - auth() {}, - exists: existsStub, - } - }, - }, - '@overleaf/metrics': { inc() {} }, - './Profiler': (Profiler = (function () { - Profiler = class Profiler { - static initClass() { - this.prototype.log = sinon.stub().returns({ end: sinon.stub() }) - this.prototype.end = sinon.stub() - } - } - Profiler.initClass() - return Profiler - })()), - } - const LockManager = SandboxedModule.require(modulePath, { requires: mocks }) - - it('should return true if the key does not exists', function (done) { - existsStub.yields(null, '0') - return LockManager.checkLock(docId, (err, free) => { - if (err) return done(err) - free.should.equal(true) - return done() - }) - }) - - return it('should return false if the key does exists', function (done) { - existsStub.yields(null, '1') - return LockManager.checkLock(docId, (err, free) => { - if (err) return done(err) - free.should.equal(false) - return done() - }) - }) -}) diff --git a/services/document-updater/test/unit/js/LockManager/ReleasingTheLock.js b/services/document-updater/test/unit/js/LockManager/ReleasingTheLock.js deleted file mode 100644 index a39a3b4fff..0000000000 --- a/services/document-updater/test/unit/js/LockManager/ReleasingTheLock.js +++ /dev/null @@ -1,94 +0,0 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS206: Consider reworking classes to avoid initClass - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const sinon = require('sinon') -const assert = require('node:assert') -const path = require('node:path') -const modulePath = path.join(__dirname, '../../../../app/js/LockManager.js') -const projectId = 1234 -const docId = 5678 -const SandboxedModule = require('sandboxed-module') - -describe('LockManager - releasing the lock', function () { - beforeEach(function () { - let Profiler - this.client = { - auth() {}, - eval: sinon.stub(), - } - const mocks = { - '@overleaf/redis-wrapper': { - createClient: () => this.client, - }, - '@overleaf/settings': { - redis: { - lock: { - key_schema: { - blockingKey({ doc_id: docId }) { - return `Blocking:${docId}` - }, - }, - }, - }, - }, - '@overleaf/metrics': { inc() {} }, - './Profiler': (Profiler = (function () { - Profiler = class Profiler { - static initClass() { - this.prototype.log = sinon.stub().returns({ end: sinon.stub() }) - this.prototype.end = sinon.stub() - } - } - Profiler.initClass() - return Profiler - })()), - } - this.LockManager = SandboxedModule.require(modulePath, { requires: mocks }) - this.lockValue = 'lock-value-stub' - return (this.callback = sinon.stub()) - }) - - describe('when the lock is current', function () { - beforeEach(function () { - this.client.eval = sinon.stub().yields(null, 1) - return this.LockManager.releaseLock(docId, this.lockValue, this.callback) - }) - - it('should clear the data from redis', function () { - return this.client.eval - .calledWith( - this.LockManager.unlockScript, - 1, - `Blocking:${docId}`, - this.lockValue - ) - .should.equal(true) - }) - - return it('should call the callback', function () { - return this.callback.called.should.equal(true) - }) - }) - - return describe('when the lock has expired', function () { - beforeEach(function () { - this.client.eval = sinon.stub().yields(null, 0) - return this.LockManager.releaseLock(docId, this.lockValue, this.callback) - }) - - return it('should return an error if the lock has expired', function () { - return this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) - }) - }) -}) diff --git a/services/document-updater/test/unit/js/LockManager/getLockTests.js b/services/document-updater/test/unit/js/LockManager/getLockTests.js deleted file mode 100644 index 938c593a51..0000000000 --- a/services/document-updater/test/unit/js/LockManager/getLockTests.js +++ /dev/null @@ -1,126 +0,0 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns - * DS206: Consider reworking classes to avoid initClass - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const sinon = require('sinon') -const modulePath = '../../../../app/js/LockManager.js' -const SandboxedModule = require('sandboxed-module') - -describe('LockManager - getting the lock', function () { - beforeEach(function () { - let Profiler - this.LockManager = SandboxedModule.require(modulePath, { - requires: { - '@overleaf/redis-wrapper': { - createClient: () => { - return { auth() {} } - }, - }, - '@overleaf/metrics': { inc() {} }, - './Profiler': (Profiler = (function () { - Profiler = class Profiler { - static initClass() { - this.prototype.log = sinon.stub().returns({ end: sinon.stub() }) - this.prototype.end = sinon.stub() - } - } - Profiler.initClass() - return Profiler - })()), - }, - }) - this.callback = sinon.stub() - return (this.doc_id = 'doc-id-123') - }) - - describe('when the lock is not set', function () { - beforeEach(function (done) { - this.lockValue = 'mock-lock-value' - this.LockManager.tryLock = sinon - .stub() - .callsArgWith(1, null, true, this.lockValue) - return this.LockManager.getLock(this.doc_id, (...args) => { - this.callback(...Array.from(args || [])) - return done() - }) - }) - - it('should try to get the lock', function () { - return this.LockManager.tryLock.calledWith(this.doc_id).should.equal(true) - }) - - it('should only need to try once', function () { - return this.LockManager.tryLock.callCount.should.equal(1) - }) - - return it('should return the callback with the lock value', function () { - return this.callback.calledWith(null, this.lockValue).should.equal(true) - }) - }) - - describe('when the lock is initially set', function () { - beforeEach(function (done) { - this.lockValue = 'mock-lock-value' - const startTime = Date.now() - let tries = 0 - this.LockManager.LOCK_TEST_INTERVAL = 5 - this.LockManager.tryLock = (docId, callback) => { - if (callback == null) { - callback = function () {} - } - if (Date.now() - startTime < 20 || tries < 2) { - tries = tries + 1 - return callback(null, false) - } else { - return callback(null, true, this.lockValue) - } - } - sinon.spy(this.LockManager, 'tryLock') - - return this.LockManager.getLock(this.doc_id, (...args) => { - this.callback(...Array.from(args || [])) - return done() - }) - }) - - it('should call tryLock multiple times until free', function () { - return (this.LockManager.tryLock.callCount > 1).should.equal(true) - }) - - return it('should return the callback with the lock value', function () { - return this.callback.calledWith(null, this.lockValue).should.equal(true) - }) - }) - - return describe('when the lock times out', function () { - beforeEach(function (done) { - const time = Date.now() - this.LockManager.MAX_LOCK_WAIT_TIME = 5 - this.LockManager.tryLock = sinon.stub().callsArgWith(1, null, false) - return this.LockManager.getLock(this.doc_id, (...args) => { - this.callback(...Array.from(args || [])) - return done() - }) - }) - - return it('should return the callback with an error', function () { - return this.callback - .calledWith( - sinon.match - .instanceOf(Error) - .and(sinon.match.has('doc_id', this.doc_id)) - ) - .should.equal(true) - }) - }) -}) diff --git a/services/document-updater/test/unit/js/LockManager/tryLockTests.js b/services/document-updater/test/unit/js/LockManager/tryLockTests.js deleted file mode 100644 index 861f733979..0000000000 --- a/services/document-updater/test/unit/js/LockManager/tryLockTests.js +++ /dev/null @@ -1,155 +0,0 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS206: Consider reworking classes to avoid initClass - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const sinon = require('sinon') -const modulePath = '../../../../app/js/LockManager.js' -const SandboxedModule = require('sandboxed-module') -const tk = require('timekeeper') - -describe('LockManager - trying the lock', function () { - beforeEach(function () { - let Profiler - this.LockManager = SandboxedModule.require(modulePath, { - requires: { - '@overleaf/redis-wrapper': { - createClient: () => { - return { - auth() {}, - set: (this.set = sinon.stub()), - } - }, - }, - '@overleaf/metrics': { inc() {} }, - '@overleaf/settings': { - redis: { - lock: { - key_schema: { - blockingKey({ doc_id: docId }) { - return `Blocking:${docId}` - }, - }, - }, - }, - }, - './Profiler': - (this.Profiler = Profiler = - (function () { - Profiler = class Profiler { - static initClass() { - this.prototype.log = sinon - .stub() - .returns({ end: sinon.stub() }) - this.prototype.end = sinon.stub() - } - } - Profiler.initClass() - return Profiler - })()), - }, - }) - - this.callback = sinon.stub() - return (this.doc_id = 'doc-id-123') - }) - - describe('when the lock is not set', function () { - beforeEach(function () { - this.lockValue = 'mock-lock-value' - this.LockManager.randomLock = sinon.stub().returns(this.lockValue) - this.set.callsArgWith(5, null, 'OK') - return this.LockManager.tryLock(this.doc_id, this.callback) - }) - - it('should set the lock key with an expiry if it is not set', function () { - return this.set - .calledWith(`Blocking:${this.doc_id}`, this.lockValue, 'EX', 30, 'NX') - .should.equal(true) - }) - - return it('should return the callback with true and the lock value', function () { - return this.callback - .calledWith(null, true, this.lockValue) - .should.equal(true) - }) - }) - - describe('when the lock is already set', function () { - beforeEach(function () { - this.set.callsArgWith(5, null, null) - return this.LockManager.tryLock(this.doc_id, this.callback) - }) - - return it('should return the callback with false', function () { - return this.callback.calledWith(null, false).should.equal(true) - }) - }) - - return describe('when it takes a long time for redis to set the lock', function () { - beforeEach(function () { - tk.freeze(Date.now()) - this.lockValue = 'mock-lock-value' - this.LockManager.randomLock = sinon.stub().returns(this.lockValue) - this.LockManager.releaseLock = sinon.stub().callsArgWith(2, null) - this.set.callsFake((_key, _v, _ex, _ttl, _nx, cb) => { - tk.freeze(Date.now() + 7000) - cb(null, 'OK') - }) - }) - after(function () { - tk.reset() - }) - - describe('in all cases', function () { - beforeEach(function () { - return this.LockManager.tryLock(this.doc_id, this.callback) - }) - - it('should set the lock key with an expiry if it is not set', function () { - return this.set - .calledWith(`Blocking:${this.doc_id}`, this.lockValue, 'EX', 30, 'NX') - .should.equal(true) - }) - - return it('should try to release the lock', function () { - return this.LockManager.releaseLock - .calledWith(this.doc_id, this.lockValue) - .should.equal(true) - }) - }) - - describe('if the lock is released successfully', function () { - beforeEach(function () { - this.LockManager.releaseLock = sinon.stub().callsArgWith(2, null) - return this.LockManager.tryLock(this.doc_id, this.callback) - }) - - return it('should return the callback with false', function () { - return this.callback.calledWith(null, false).should.equal(true) - }) - }) - - return describe('if the lock has already timed out', function () { - beforeEach(function () { - this.LockManager.releaseLock = sinon - .stub() - .callsArgWith(2, new Error('tried to release timed out lock')) - return this.LockManager.tryLock(this.doc_id, this.callback) - }) - - return it('should return the callback with an error', function () { - return this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) - }) - }) - }) -}) diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index faa18df8d7..2a6d067469 100644 --- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js +++ b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js @@ -19,9 +19,6 @@ describe('RedisManager', function () { redis: { documentupdater: { key_schema: { - blockingKey({ doc_id: docId }) { - return `Blocking:${docId}` - }, docLines({ doc_id: docId }) { return `doclines:${docId}` }, diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 4e8b4ab521..64da79adbc 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js @@ -10,7 +10,6 @@ describe('UpdateManager', function () { this.project_id = 'project-id-123' this.projectHistoryId = 'history-id-123' this.doc_id = 'document-id-123' - this.lockValue = 'mock-lock-value' this.projectLockValue = 'mock-project-lock-value' this.pathname = '/a/b/c.tex' @@ -24,16 +23,10 @@ describe('UpdateManager', function () { this.Profiler.prototype.log = sinon.stub().returns({ end: sinon.stub() }) this.Profiler.prototype.end = sinon.stub() - this.LockManager = { - promises: { - getLock: sinon.stub().resolves(this.lockValue), - releaseLock: sinon.stub().resolves(), - }, - } - this.ProjectLockManager = { promises: { getLock: sinon.stub().resolves(this.projectLockValue), + extendLock: sinon.stub().resolves(), releaseLock: sinon.stub().resolves(), }, } @@ -101,7 +94,6 @@ describe('UpdateManager', function () { this.UpdateManager = SandboxedModule.require(MODULE_PATH, { requires: { - './LockManager': this.LockManager, './ProjectLockManager': this.ProjectLockManager, './RedisManager': this.RedisManager, './RealTimeRedisManager': this.RealTimeRedisManager, @@ -872,15 +864,18 @@ describe('UpdateManager', function () { ) }) - it('should lock the project then the doc', function () { + it('should lock the project', function () { this.ProjectLockManager.promises.getLock .calledWith(this.project_id) .should.equal(true) - this.LockManager.promises.getLock - .calledWith(this.doc_id) + }) + + it('should refresh the project lock before running the method', function () { + this.ProjectLockManager.promises.extendLock + .calledWith(this.project_id, this.projectLockValue) .should.equal(true) - this.LockManager.promises.getLock - .calledAfter(this.ProjectLockManager.promises.getLock) + this.method + .calledAfter(this.ProjectLockManager.promises.extendLock) .should.equal(true) }) @@ -900,10 +895,7 @@ describe('UpdateManager', function () { expect(this.response).to.equal(this.methodResult) }) - it('should release both locks', function () { - this.LockManager.promises.releaseLock - .calledWith(this.doc_id, this.lockValue) - .should.equal(true) + it('should release the project lock', function () { this.ProjectLockManager.promises.releaseLock .calledWith(this.project_id, this.projectLockValue) .should.equal(true) @@ -956,10 +948,7 @@ describe('UpdateManager', function () { ).to.be.rejectedWith(this.error) }) - it('should free both locks', function () { - this.LockManager.promises.releaseLock - .calledWith(this.doc_id, this.lockValue) - .should.equal(true) + it('should free the project lock', function () { this.ProjectLockManager.promises.releaseLock .calledWith(this.project_id, this.projectLockValue) .should.equal(true)