From c530b791a4efe5481cad4f0820c6f10d71ea28a5 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 11 Jan 2024 08:13:11 +0000 Subject: [PATCH] Merge pull request #16471 from overleaf/em-clsi-in-memory-lock Replace filesystem lock in CLSI with in-memory lock GitOrigin-RevId: de1ac3beca67bb4e9070806871a1c7b6a59aa77f --- package-lock.json | 4 +- services/clsi/app/js/CompileManager.js | 8 +- services/clsi/app/js/LockManager.js | 70 +++++++----------- services/clsi/app/js/OutputFileFinder.js | 1 - services/clsi/package.json | 1 - .../clsi/test/unit/js/CompileManagerTests.js | 6 +- .../clsi/test/unit/js/LockManagerTests.js | 73 ++++++++----------- 7 files changed, 66 insertions(+), 97 deletions(-) diff --git a/package-lock.json b/package-lock.json index 5d33ca3dde..564c805874 100644 --- a/package-lock.json +++ b/package-lock.json @@ -30400,6 +30400,7 @@ "version": "1.0.4", "resolved": "https://registry.npmjs.org/lockfile/-/lockfile-1.0.4.tgz", "integrity": "sha512-cvbTwETRfsFh4nHsL1eGWapU1XFi5Ot9E85sWAwia7Y7EgB7vfqcZhTKZ+l7hCGxSPoushMv5GKhT5PdLv03WA==", + "dev": true, "dependencies": { "signal-exit": "^3.0.2" } @@ -43511,7 +43512,6 @@ "diskusage": "^1.1.3", "dockerode": "^3.1.0", "express": "^4.18.2", - "lockfile": "^1.0.4", "lodash": "^4.17.21", "p-limit": "^3.1.0", "request": "^2.88.2", @@ -53813,7 +53813,6 @@ "diskusage": "^1.1.3", "dockerode": "^3.1.0", "express": "^4.18.2", - "lockfile": "^1.0.4", "lodash": "^4.17.21", "mocha": "^10.2.0", "mock-fs": "^5.1.2", @@ -73612,6 +73611,7 @@ "version": "1.0.4", "resolved": "https://registry.npmjs.org/lockfile/-/lockfile-1.0.4.tgz", "integrity": "sha512-cvbTwETRfsFh4nHsL1eGWapU1XFi5Ot9E85sWAwia7Y7EgB7vfqcZhTKZ+l7hCGxSPoushMv5GKhT5PdLv03WA==", + "dev": true, "requires": { "signal-exit": "^3.0.2" } diff --git a/services/clsi/app/js/CompileManager.js b/services/clsi/app/js/CompileManager.js index e59f89ba92..c4c40c3e1f 100644 --- a/services/clsi/app/js/CompileManager.js +++ b/services/clsi/app/js/CompileManager.js @@ -44,15 +44,13 @@ function getOutputDir(projectId, userId) { async function doCompileWithLock(request) { const compileDir = getCompileDir(request.project_id, request.user_id) - // use a .project-lock file in the compile directory to prevent - // simultaneous compiles - const lockFile = Path.join(compileDir, '.project-lock') await fsPromises.mkdir(compileDir, { recursive: true }) - const lock = await LockManager.acquire(lockFile) + // prevent simultaneous compiles + const lock = LockManager.acquire(compileDir) try { return await doCompile(request) } finally { - await lock.release() + lock.release() } } diff --git a/services/clsi/app/js/LockManager.js b/services/clsi/app/js/LockManager.js index f73138dcb2..0ecd04356e 100644 --- a/services/clsi/app/js/LockManager.js +++ b/services/clsi/app/js/LockManager.js @@ -1,60 +1,44 @@ -const { promisify } = require('util') -const OError = require('@overleaf/o-error') -const Lockfile = require('lockfile') +const logger = require('@overleaf/logger') const Errors = require('./Errors') -const fsPromises = require('fs/promises') -const Path = require('path') +const RequestParser = require('./RequestParser') -const LOCK_OPTS = { - pollPeriod: 1000, // 1s between each test of the lock - wait: 15000, // 15s maximum time to spend trying to get the lock - stale: 5 * 60 * 1000, // 5 mins time until lock auto expires -} +// The lock timeout should be higher than the maximum end-to-end compile time. +// Here, we use the maximum compile timeout plus 2 minutes. +const LOCK_TIMEOUT_MS = RequestParser.MAX_TIMEOUT * 1000 + 120000 -const PromisifiedLockfile = { - lock: promisify(Lockfile.lock), - unlock: promisify(Lockfile.unlock), -} +const LOCKS = new Map() -async function acquire(path) { - try { - await PromisifiedLockfile.lock(path, LOCK_OPTS) - } catch (err) { - if (err.code === 'EEXIST') { - throw new Errors.AlreadyCompilingError('compile in progress') +function acquire(key) { + const currentLock = LOCKS.get(key) + if (currentLock != null) { + if (currentLock.isExpired()) { + logger.warn({ key }, 'Compile lock expired') + currentLock.release() } else { - const dir = Path.dirname(path) - const [statLock, statDir, readdirDir] = await Promise.allSettled([ - fsPromises.lstat(path), - fsPromises.lstat(dir), - fsPromises.readdir(dir), - ]) - OError.tag(err, 'unable to get lock', { - statLock: unwrapPromiseResult(statLock), - statDir: unwrapPromiseResult(statDir), - readdirDir: unwrapPromiseResult(readdirDir), - }) - throw err + throw new Errors.AlreadyCompilingError('compile in progress') } } - return new Lock(path) + + const lock = new Lock(key) + LOCKS.set(key, lock) + return lock } class Lock { - constructor(path) { - this._path = path + constructor(key) { + this.key = key + this.expiresAt = Date.now() + LOCK_TIMEOUT_MS } - async release() { - await PromisifiedLockfile.unlock(this._path) + isExpired() { + return Date.now() >= this.expiresAt } -} -function unwrapPromiseResult(result) { - if (result.status === 'fulfilled') { - return result.value - } else { - return result.reason + release() { + const lockWasActive = LOCKS.delete(this.key) + if (!lockWasActive) { + logger.error({ key: this.key }, 'Lock was released twice') + } } } diff --git a/services/clsi/app/js/OutputFileFinder.js b/services/clsi/app/js/OutputFileFinder.js index b860b87817..8ca13183dc 100644 --- a/services/clsi/app/js/OutputFileFinder.js +++ b/services/clsi/app/js/OutputFileFinder.js @@ -31,7 +31,6 @@ async function findOutputFiles(resources, directory) { for (const path of files) { if (incomingResources.has(path)) continue if (path === '.project-sync-state') continue - if (path === '.project-lock') continue outputFiles.push({ path, type: Path.extname(path).replace(/^\./, '') || undefined, diff --git a/services/clsi/package.json b/services/clsi/package.json index 16962dc1e9..debab724a3 100644 --- a/services/clsi/package.json +++ b/services/clsi/package.json @@ -28,7 +28,6 @@ "diskusage": "^1.1.3", "dockerode": "^3.1.0", "express": "^4.18.2", - "lockfile": "^1.0.4", "lodash": "^4.17.21", "p-limit": "^3.1.0", "request": "^2.88.2", diff --git a/services/clsi/test/unit/js/CompileManagerTests.js b/services/clsi/test/unit/js/CompileManagerTests.js index c5aacc95c3..09311a6f6f 100644 --- a/services/clsi/test/unit/js/CompileManagerTests.js +++ b/services/clsi/test/unit/js/CompileManagerTests.js @@ -99,10 +99,10 @@ describe('CompileManager', function () { }, } this.lock = { - release: sinon.stub().resolves(), + release: sinon.stub(), } this.LockManager = { - acquire: sinon.stub().resolves(this.lock), + acquire: sinon.stub().returns(this.lock), } this.SynctexOutputParser = { parseViewOutput: sinon.stub(), @@ -173,7 +173,7 @@ describe('CompileManager', function () { describe('when the project is locked', function () { beforeEach(async function () { const error = new Error('locked') - this.LockManager.acquire.rejects(error) + this.LockManager.acquire.throws(error) await expect( this.CompileManager.promises.doCompileWithLock(this.request) ).to.be.rejectedWith(error) diff --git a/services/clsi/test/unit/js/LockManagerTests.js b/services/clsi/test/unit/js/LockManagerTests.js index 048387067c..772849ddbc 100644 --- a/services/clsi/test/unit/js/LockManagerTests.js +++ b/services/clsi/test/unit/js/LockManagerTests.js @@ -1,71 +1,60 @@ const { expect } = require('chai') const sinon = require('sinon') -const mockFs = require('mock-fs') -const OError = require('@overleaf/o-error') const LockManager = require('../../../app/js/LockManager') const Errors = require('../../../app/js/Errors') describe('LockManager', function () { beforeEach(function () { - this.lockFile = '/local/compile/directory/.project-lock' - mockFs({ - '/local/compile/directory': {}, - }) + this.key = '/local/compile/directory' this.clock = sinon.useFakeTimers() }) afterEach(function () { - mockFs.restore() this.clock.restore() }) describe('when the lock is available', function () { - it('the lock can be acquired', async function () { - await LockManager.acquire(this.lockFile) - }) - - it('acquiring a lock in a nonexistent directory throws an error with debug info', async function () { - const err = await expect( - LockManager.acquire('/invalid/path/.project-lock') - ).to.be.rejected - const info = OError.getFullInfo(err) - expect(info).to.have.keys(['statLock', 'statDir', 'readdirDir']) - expect(info.statLock.code).to.equal('ENOENT') - expect(info.statDir.code).to.equal('ENOENT') - expect(info.readdirDir.code).to.equal('ENOENT') + it('the lock can be acquired', function () { + const lock = LockManager.acquire(this.key) + expect(lock).to.exist + lock.release() }) }) describe('after the lock is acquired', function () { - beforeEach(async function () { - this.lock = await LockManager.acquire(this.lockFile) + beforeEach(function () { + this.lock = LockManager.acquire(this.key) }) - it("the lock can't be acquired again", function (done) { - const promise = LockManager.acquire(this.lockFile) - // runAllAsync() will advance through time until there are no pending - // timers or promises. It interferes with Mocha's promise interface, so - // we use Mocha's callback interface for this test. - this.clock.runAllAsync() - expect(promise) - .to.be.rejectedWith(Errors.AlreadyCompilingError) - .then(() => { - done() - }) - .catch(err => { - done(err) - }) + afterEach(function () { + if (this.lock != null) { + this.lock.release() + } }) - it('the lock can be acquired again after an expiry period', async function () { - // The expiry time is 5 minutes. Let's wait 10 minutes. - this.clock.tick(10 * 60 * 1000) - await LockManager.acquire(this.lockFile) + it("the lock can't be acquired again", function () { + expect(() => LockManager.acquire(this.key)).to.throw( + Errors.AlreadyCompilingError + ) }) - it('the lock can be acquired again after it was released', async function () { + it('another lock can be acquired', function () { + const lock = LockManager.acquire('another key') + expect(lock).to.exist + lock.release() + }) + + it('the lock can be acquired again after an expiry period', function () { + // The expiry time is a little bit over 10 minutes. Let's wait 15 minutes. + this.clock.tick(15 * 60 * 1000) + this.lock = LockManager.acquire(this.key) + expect(this.lock).to.exist + }) + + it('the lock can be acquired again after it was released', function () { this.lock.release() - await LockManager.acquire(this.lockFile) + this.lock = LockManager.acquire(this.key) + expect(this.lock).to.exist }) }) })