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

@@ -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"
       }

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()
   }
 }
 

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')
+    }
   }
 }
 

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,

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",

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)

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)
+    })
+
+    afterEach(function () {
+      if (this.lock != null) {
+        this.lock.release()
+      }
+    })
+
+    it("the lock can't be acquired again", function () {
+      expect(() => LockManager.acquire(this.key)).to.throw(
+        Errors.AlreadyCompilingError
+      )
     })
 
-    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)
-        })
+    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', 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 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', async function () {
+    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
     })
   })
 })