Merge pull request #8714 from overleaf/em-promisify-compile-manager

Promisify CompileManager

GitOrigin-RevId: 644ed061ae139d6196b24f8ead38579de6b844a3

package-lock.json

@@ -31888,6 +31888,7 @@
         "chai": "^4.3.6",
         "chai-as-promised": "^7.1.1",
         "mocha": "^8.4.0",
+        "mock-fs": "^5.1.2",
         "sandboxed-module": "^2.0.4",
         "sinon": "~9.0.1",
         "sinon-chai": "^3.7.0",
@@ -39708,6 +39709,7 @@
         "lockfile": "^1.0.4",
         "lodash": "^4.17.21",
         "mocha": "^8.4.0",
+        "mock-fs": "^5.1.2",
         "p-limit": "^3.1.0",
         "pdfjs-dist": "~2.7.570",
         "request": "^2.88.2",

services/clsi/app/js/CompileController.js

@@ -29,93 +29,96 @@ function compile(req, res, next) {
         if (error) {
           return next(error)
         }
-        CompileManager.doCompileWithLock(
+        CompileManager.doCompileWithLock(request, (error, result) => {
-          request,
+          let { outputFiles, stats, timings } = result || {}
-          function (error, outputFiles, stats, timings) {
+          let code, status
-            let code, status
+          if (outputFiles == null) {
-            if (outputFiles == null) {
+            outputFiles = []
-              outputFiles = []
-            }
-            if (error instanceof Errors.AlreadyCompilingError) {
-              code = 423 // Http 423 Locked
-              status = 'compile-in-progress'
-            } else if (error instanceof Errors.FilesOutOfSyncError) {
-              code = 409 // Http 409 Conflict
-              status = 'retry'
-            } else if (error?.code === 'EPIPE') {
-              // docker returns EPIPE when shutting down
-              code = 503 // send 503 Unavailable response
-              status = 'unavailable'
-            } else if (error?.terminated) {
-              status = 'terminated'
-            } else if (error?.validate) {
-              status = `validation-${error.validate}`
-            } else if (error?.timedout) {
-              status = 'timedout'
-              logger.debug(
-                { err: error, project_id: request.project_id },
-                'timeout running compile'
-              )
-            } else if (error) {
-              status = 'error'
-              code = 500
-              logger.warn(
-                { err: error, project_id: request.project_id },
-                'error running compile'
-              )
-            } else {
-              if (
-                outputFiles.some(
-                  file => file.path === 'output.pdf' && file.size > 0
-                )
-              ) {
-                status = 'success'
-                lastSuccessfulCompileTimestamp = Date.now()
-              } else if (request.stopOnFirstError) {
-                status = 'stopped-on-first-error'
-              } else {
-                status = 'failure'
-                logger.warn(
-                  { project_id: request.project_id, outputFiles },
-                  'project failed to compile successfully, no output.pdf generated'
-                )
-              }
-
-              // log an error if any core files are found
-              if (outputFiles.some(file => file.path === 'core')) {
-                logger.error(
-                  { project_id: request.project_id, req, outputFiles },
-                  'core file found in output'
-                )
-              }
-            }
-
-            if (error) {
-              outputFiles = error.outputFiles || []
-            }
-
-            timer.done()
-            res.status(code || 200).send({
-              compile: {
-                status,
-                error: error?.message || error,
-                stats,
-                timings,
-                outputUrlPrefix: Settings.apis.clsi.outputUrlPrefix,
-                outputFiles: outputFiles.map(file => ({
-                  url:
-                    `${Settings.apis.clsi.url}/project/${request.project_id}` +
-                    (request.user_id != null
-                      ? `/user/${request.user_id}`
-                      : '') +
-                    (file.build != null ? `/build/${file.build}` : '') +
-                    `/output/${file.path}`,
-                  ...file,
-                })),
-              },
-            })
           }
-        )
+          if (error instanceof Errors.AlreadyCompilingError) {
+            code = 423 // Http 423 Locked
+            status = 'compile-in-progress'
+          } else if (error instanceof Errors.FilesOutOfSyncError) {
+            code = 409 // Http 409 Conflict
+            status = 'retry'
+            logger.warn(
+              {
+                projectId: request.project_id,
+                userId: request.user_id,
+              },
+              'files out of sync, please retry'
+            )
+          } else if (error?.code === 'EPIPE') {
+            // docker returns EPIPE when shutting down
+            code = 503 // send 503 Unavailable response
+            status = 'unavailable'
+          } else if (error?.terminated) {
+            status = 'terminated'
+          } else if (error?.validate) {
+            status = `validation-${error.validate}`
+          } else if (error?.timedout) {
+            status = 'timedout'
+            logger.debug(
+              { err: error, projectId: request.project_id },
+              'timeout running compile'
+            )
+          } else if (error) {
+            status = 'error'
+            code = 500
+            logger.error(
+              { err: error, projectId: request.project_id },
+              'error running compile'
+            )
+          } else {
+            if (
+              outputFiles.some(
+                file => file.path === 'output.pdf' && file.size > 0
+              )
+            ) {
+              status = 'success'
+              lastSuccessfulCompileTimestamp = Date.now()
+            } else if (request.stopOnFirstError) {
+              status = 'stopped-on-first-error'
+            } else {
+              status = 'failure'
+              logger.warn(
+                { projectId: request.project_id, outputFiles },
+                'project failed to compile successfully, no output.pdf generated'
+              )
+            }
+
+            // log an error if any core files are found
+            if (outputFiles.some(file => file.path === 'core')) {
+              logger.error(
+                { projectId: request.project_id, req, outputFiles },
+                'core file found in output'
+              )
+            }
+          }
+
+          if (error) {
+            outputFiles = error.outputFiles || []
+          }
+
+          timer.done()
+          res.status(code || 200).send({
+            compile: {
+              status,
+              error: error?.message || error,
+              stats,
+              timings,
+              outputUrlPrefix: Settings.apis.clsi.outputUrlPrefix,
+              outputFiles: outputFiles.map(file => ({
+                url:
+                  `${Settings.apis.clsi.url}/project/${request.project_id}` +
+                  (request.user_id != null ? `/user/${request.user_id}` : '') +
+                  (file.build != null ? `/build/${file.build}` : '') +
+                  `/output/${file.path}`,
+                ...file,
+              })),
+            },
+          })
+        })
       }
     )
   })

services/clsi/app/js/DockerRunner.js

@@ -1,3 +1,4 @@
+const { promisify } = require('util')
 const Settings = require('@overleaf/settings')
 const logger = require('@overleaf/logger')
 const Docker = require('dockerode')
@@ -617,3 +618,7 @@ const DockerRunner = {
 DockerRunner.startContainerMonitor()
 
 module.exports = DockerRunner
+module.exports.promises = {
+  run: promisify(DockerRunner.run),
+  kill: promisify(DockerRunner.kill),
+}

services/clsi/app/js/DraftModeManager.js

@@ -12,6 +12,7 @@
  */
 let DraftModeManager
 const fs = require('fs')
+const { promisify } = require('util')
 const logger = require('@overleaf/logger')
 
 module.exports = DraftModeManager = {
@@ -54,3 +55,7 @@ module.exports = DraftModeManager = {
     )
   },
 }
+
+module.exports.promises = {
+  injectDraftMode: promisify(DraftModeManager.injectDraftMode),
+}

services/clsi/app/js/LatexRunner.js

@@ -1,4 +1,5 @@
 const Path = require('path')
+const { promisify } = require('util')
 const Settings = require('@overleaf/settings')
 const logger = require('@overleaf/logger')
 const CommandRunner = require('./CommandRunner')
@@ -192,4 +193,17 @@ function _buildLatexCommand(mainFile, opts = {}) {
 module.exports = {
   runLatex,
   killLatex,
+  promises: {
+    runLatex: (projectId, options) =>
+      new Promise((resolve, reject) => {
+        runLatex(projectId, options, (err, output, stats, timing) => {
+          if (err) {
+            reject(err)
+          } else {
+            resolve({ output, stats, timing })
+          }
+        })
+      }),
+    killLatex: promisify(killLatex),
+  },
 }

services/clsi/app/js/LocalCommandRunner.js

@@ -14,6 +14,7 @@
  */
 let CommandRunner
 const { spawn } = require('child_process')
+const { promisify } = require('util')
 const _ = require('lodash')
 const logger = require('@overleaf/logger')
 
@@ -100,3 +101,8 @@ module.exports = CommandRunner = {
     return callback()
   },
 }
+
+module.exports.promises = {
+  run: promisify(CommandRunner.run),
+  kill: promisify(CommandRunner.kill),
+}

services/clsi/app/js/LockManager.js

@@ -1,71 +1,61 @@
-/* eslint-disable
+const { promisify } = require('util')
-    no-unused-vars,
+const OError = require('@overleaf/o-error')
-*/
+const Lockfile = require('lockfile')
-// 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
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-let LockManager
-const Settings = require('@overleaf/settings')
-const logger = require('@overleaf/logger')
-const Lockfile = require('lockfile') // from https://github.com/npm/lockfile
 const Errors = require('./Errors')
-const fs = require('fs')
+const fsPromises = require('fs/promises')
 const Path = require('path')
-module.exports = LockManager = {
-  LOCK_TEST_INTERVAL: 1000, // 50ms between each test of the lock
-  MAX_LOCK_WAIT_TIME: 15000, // 10s maximum time to spend trying to get the lock
-  LOCK_STALE: 5 * 60 * 1000, // 5 mins time until lock auto expires
 
-  runWithLock(path, runner, callback) {
+const LOCK_OPTS = {
-    if (callback == null) {
+  pollPeriod: 1000, // 1s between each test of the lock
-      callback = function () {}
+  wait: 15000, // 15s maximum time to spend trying to get the lock
-    }
+  stale: 5 * 60 * 1000, // 5 mins time until lock auto expires
-    const lockOpts = {
-      wait: this.MAX_LOCK_WAIT_TIME,
-      pollPeriod: this.LOCK_TEST_INTERVAL,
-      stale: this.LOCK_STALE,
-    }
-    return Lockfile.lock(path, lockOpts, function (error) {
-      if ((error != null ? error.code : undefined) === 'EEXIST') {
-        return callback(new Errors.AlreadyCompilingError('compile in progress'))
-      } else if (error != null) {
-        return fs.lstat(path, (statLockErr, statLock) =>
-          fs.lstat(Path.dirname(path), (statDirErr, statDir) =>
-            fs.readdir(Path.dirname(path), function (readdirErr, readdirDir) {
-              logger.err(
-                {
-                  error,
-                  path,
-                  statLock,
-                  statLockErr,
-                  statDir,
-                  statDirErr,
-                  readdirErr,
-                  readdirDir,
-                },
-                'unable to get lock'
-              )
-              return callback(error)
-            })
-          )
-        )
-      } else {
-        return runner((error1, ...args) =>
-          Lockfile.unlock(path, function (error2) {
-            error = error1 || error2
-            if (error != null) {
-              return callback(error)
-            }
-            return callback(null, ...Array.from(args))
-          })
-        )
-      }
-    })
-  },
 }
+
+const PromisifiedLockfile = {
+  lock: promisify(Lockfile.lock),
+  unlock: promisify(Lockfile.unlock),
+}
+
+async function acquire(path) {
+  try {
+    await PromisifiedLockfile.lock(path, LOCK_OPTS)
+  } catch (err) {
+    if (err.code === 'EEXIST') {
+      throw new Errors.AlreadyCompilingError('compile in progress')
+    } 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
+    }
+  }
+  return new Lock(path)
+}
+
+class Lock {
+  constructor(path) {
+    this._path = path
+  }
+
+  async release() {
+    await PromisifiedLockfile.unlock(this._path)
+  }
+}
+
+function unwrapPromiseResult(result) {
+  if (result.status === 'fulfilled') {
+    return result.value
+  } else {
+    return result.reason
+  }
+}
+
+module.exports = { acquire }

services/clsi/app/js/OutputCacheManager.js

@@ -695,6 +695,7 @@ function __guard__(value, transform) {
 
 OutputCacheManager.promises = {
   expireOutputFiles: promisify(OutputCacheManager.expireOutputFiles),
+  saveOutputFiles: promisify(OutputCacheManager.saveOutputFiles),
   saveOutputFilesInBuildDir: promisify(
     OutputCacheManager.saveOutputFilesInBuildDir
   ),

services/clsi/app/js/OutputFileFinder.js

@@ -76,3 +76,20 @@ module.exports = OutputFileFinder = {
     })
   },
 }
+
+module.exports.promises = {
+  findOutputFiles: (resources, directory) =>
+    new Promise((resolve, reject) => {
+      OutputFileFinder.findOutputFiles(
+        resources,
+        directory,
+        (err, outputFiles, allFiles) => {
+          if (err) {
+            reject(err)
+          } else {
+            resolve({ outputFiles, allFiles })
+          }
+        }
+      )
+    }),
+}

services/clsi/app/js/ResourceWriter.js

@@ -14,6 +14,7 @@
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
 let ResourceWriter
+const { promisify } = require('util')
 const UrlCache = require('./UrlCache')
 const Path = require('path')
 const fs = require('fs')
@@ -85,22 +86,26 @@ module.exports = ResourceWriter = {
       if (error != null) {
         return callback(error)
       }
-      this.saveAllResourcesToDisk(request, basePath, function (error) {
+      ResourceWriter.saveAllResourcesToDisk(
-        if (error != null) {
+        request,
-          return callback(error)
+        basePath,
-        }
+        function (error) {
-        return ResourceStateManager.saveProjectState(
+          if (error != null) {
-          request.syncState,
+            return callback(error)
-          request.resources,
-          basePath,
-          function (error) {
-            if (error != null) {
-              return callback(error)
-            }
-            return callback(null, request.resources)
           }
-        )
+          return ResourceStateManager.saveProjectState(
-      })
+            request.syncState,
+            request.resources,
+            basePath,
+            function (error) {
+              if (error != null) {
+                return callback(error)
+              }
+              return callback(null, request.resources)
+            }
+          )
+        }
+      )
     })
   },
 
@@ -108,14 +113,19 @@ module.exports = ResourceWriter = {
     if (callback == null) {
       callback = function () {}
     }
-    return this._createDirectory(basePath, error => {
+    return ResourceWriter._createDirectory(basePath, error => {
       if (error != null) {
         return callback(error)
       }
       const jobs = Array.from(resources).map(resource =>
         (resource => {
           return callback =>
-            this._writeResourceToDisk(project_id, resource, basePath, callback)
+            ResourceWriter._writeResourceToDisk(
+              project_id,
+              resource,
+              basePath,
+              callback
+            )
         })(resource)
       )
       return async.parallelLimit(jobs, parallelFileDownloads, callback)
@@ -126,28 +136,33 @@ module.exports = ResourceWriter = {
     if (callback == null) {
       callback = function () {}
     }
-    return this._createDirectory(basePath, error => {
+    return ResourceWriter._createDirectory(basePath, error => {
       if (error != null) {
         return callback(error)
       }
       const { project_id, resources } = request
-      this._removeExtraneousFiles(request, resources, basePath, error => {
+      ResourceWriter._removeExtraneousFiles(
-        if (error != null) {
+        request,
-          return callback(error)
+        resources,
+        basePath,
+        error => {
+          if (error != null) {
+            return callback(error)
+          }
+          const jobs = Array.from(resources).map(resource =>
+            (resource => {
+              return callback =>
+                ResourceWriter._writeResourceToDisk(
+                  project_id,
+                  resource,
+                  basePath,
+                  callback
+                )
+            })(resource)
+          )
+          return async.parallelLimit(jobs, parallelFileDownloads, callback)
         }
-        const jobs = Array.from(resources).map(resource =>
+      )
-          (resource => {
-            return callback =>
-              this._writeResourceToDisk(
-                project_id,
-                resource,
-                basePath,
-                callback
-              )
-          })(resource)
-        )
-        return async.parallelLimit(jobs, parallelFileDownloads, callback)
-      })
     })
   },
 
@@ -356,3 +371,12 @@ module.exports = ResourceWriter = {
     }
   },
 }
+
+module.exports.promises = {
+  syncResourcesToDisk: promisify(ResourceWriter.syncResourcesToDisk),
+  saveIncrementalResourcesToDisk: promisify(
+    ResourceWriter.saveIncrementalResourcesToDisk
+  ),
+  saveAllResourcesToDisk: promisify(ResourceWriter.saveAllResourcesToDisk),
+  checkPath: promisify(ResourceWriter.checkPath),
+}

services/clsi/app/js/TikzManager.js

@@ -13,6 +13,7 @@
 let TikzManager
 const fs = require('fs')
 const Path = require('path')
+const { promisify } = require('util')
 const ResourceWriter = require('./ResourceWriter')
 const SafeReader = require('./SafeReader')
 const logger = require('@overleaf/logger')
@@ -101,3 +102,8 @@ module.exports = TikzManager = {
     )
   },
 }
+
+module.exports.promises = {
+  checkMainFile: promisify(TikzManager.checkMainFile),
+  injectOutputFile: promisify(TikzManager.injectOutputFile),
+}

services/clsi/package.json

@@ -39,6 +39,7 @@
     "chai": "^4.3.6",
     "chai-as-promised": "^7.1.1",
     "mocha": "^8.4.0",
+    "mock-fs": "^5.1.2",
     "sandboxed-module": "^2.0.4",
     "sinon": "~9.0.1",
     "sinon-chai": "^3.7.0",

services/clsi/test/setup.js

@@ -1,10 +1,12 @@
 const chai = require('chai')
 const sinonChai = require('sinon-chai')
+const chaiAsPromised = require('chai-as-promised')
 const SandboxedModule = require('sandboxed-module')
 
 // Setup chai
 chai.should()
 chai.use(sinonChai)
+chai.use(chaiAsPromised)
 
 // Global SandboxedModule settings
 SandboxedModule.configure({

services/clsi/test/unit/js/CompileControllerTests.js

@@ -111,9 +111,11 @@ describe('CompileController', function () {
 
     describe('successfully', function () {
       beforeEach(function () {
-        this.CompileManager.doCompileWithLock = sinon
+        this.CompileManager.doCompileWithLock = sinon.stub().yields(null, {
-          .stub()
+          outputFiles: this.output_files,
-          .yields(null, this.output_files, this.stats, this.timings)
+          stats: this.stats,
+          timings: this.timings,
+        })
         this.CompileController.compile(this.req, this.res)
       })
 
@@ -156,9 +158,11 @@ describe('CompileController', function () {
     describe('without a outputUrlPrefix', function () {
       beforeEach(function () {
         this.Settings.apis.clsi.outputUrlPrefix = ''
-        this.CompileManager.doCompileWithLock = sinon
+        this.CompileManager.doCompileWithLock = sinon.stub().yields(null, {
-          .stub()
+          outputFiles: this.output_files,
-          .yields(null, this.output_files, this.stats, this.timings)
+          stats: this.stats,
+          timings: this.timings,
+        })
         this.CompileController.compile(this.req, this.res)
       })
 
@@ -196,9 +200,11 @@ describe('CompileController', function () {
             build: 1234,
           },
         ]
-        this.CompileManager.doCompileWithLock = sinon
+        this.CompileManager.doCompileWithLock = sinon.stub().yields(null, {
-          .stub()
+          outputFiles: this.output_files,
-          .yields(null, this.output_files, this.stats, this.timings)
+          stats: this.stats,
+          timings: this.timings,
+        })
         this.CompileController.compile(this.req, this.res)
       })
 
@@ -237,9 +243,11 @@ describe('CompileController', function () {
             build: 1234,
           },
         ]
-        this.CompileManager.doCompileWithLock = sinon
+        this.CompileManager.doCompileWithLock = sinon.stub().yields(null, {
-          .stub()
+          outputFiles: this.output_files,
-          .yields(null, this.output_files, this.stats, this.timings)
+          stats: this.stats,
+          timings: this.timings,
+        })
         this.CompileController.compile(this.req, this.res)
       })
 

services/clsi/test/unit/js/CompileManagerTests.js

@@ -1,14 +1,14 @@
 const SandboxedModule = require('sandboxed-module')
+const { expect } = require('chai')
 const sinon = require('sinon')
-const modulePath = require('path').join(
+
+const MODULE_PATH = require('path').join(
   __dirname,
   '../../../app/js/CompileManager'
 )
-const { EventEmitter } = require('events')
 
 describe('CompileManager', function () {
   beforeEach(function () {
-    this.callback = sinon.stub()
     this.projectId = 'project-id-123'
     this.userId = '1234'
     this.resources = 'mock-resources'
@@ -40,22 +40,25 @@ describe('CompileManager', function () {
     this.compileDir = `${this.compileBaseDir}/${this.projectId}-${this.userId}`
     this.outputDir = `${this.outputBaseDir}/${this.projectId}-${this.userId}`
 
-    this.proc = new EventEmitter()
-    this.proc.stdout = new EventEmitter()
-    this.proc.stderr = new EventEmitter()
-    this.proc.stderr.setEncoding = sinon.stub().returns(this.proc.stderr)
-
     this.LatexRunner = {
-      runLatex: sinon.stub().yields(),
+      promises: {
+        runLatex: sinon.stub().resolves({}),
+      },
     }
     this.ResourceWriter = {
-      syncResourcesToDisk: sinon.stub().yields(null, this.resources),
+      promises: {
+        syncResourcesToDisk: sinon.stub().resolves(this.resources),
+      },
     }
     this.OutputFileFinder = {
-      findOutputFiles: sinon.stub().yields(null, this.outputFiles),
+      promises: {
+        findOutputFiles: sinon.stub().resolves(this.outputFiles),
+      },
     }
     this.OutputCacheManager = {
-      saveOutputFiles: sinon.stub().yields(null, this.buildFiles),
+      promises: {
+        saveOutputFiles: sinon.stub().resolves(this.buildFiles),
+      },
     }
     this.Settings = {
       path: {
@@ -74,37 +77,44 @@ describe('CompileManager', function () {
       .returns(this.compileDir)
     this.child_process = {
       exec: sinon.stub(),
-      spawn: sinon.stub().returns(this.proc),
+      execFile: sinon.stub().yields(),
     }
     this.CommandRunner = {
-      run: sinon.stub().yields(null, { stdout: this.commandOutput }),
+      promises: {
+        run: sinon.stub().resolves({ stdout: this.commandOutput }),
+      },
     }
     this.DraftModeManager = {
-      injectDraftMode: sinon.stub().yields(),
+      promises: {
+        injectDraftMode: sinon.stub().resolves(),
+      },
     }
     this.TikzManager = {
-      checkMainFile: sinon.stub().yields(null, false),
+      promises: {
+        checkMainFile: sinon.stub().resolves(false),
+      },
+    }
+    this.lock = {
+      release: sinon.stub().resolves(),
     }
     this.LockManager = {
-      runWithLock: sinon.stub().callsFake((lockFile, runner, callback) => {
+      acquire: sinon.stub().resolves(this.lock),
-        runner((err, ...result) => callback(err, ...result))
-      }),
     }
     this.SynctexOutputParser = {
       parseViewOutput: sinon.stub(),
       parseEditOutput: sinon.stub(),
     }
 
-    this.fs = {
+    this.fsPromises = {
       lstat: sinon.stub(),
       stat: sinon.stub(),
       readFile: sinon.stub(),
     }
     this.fse = {
-      ensureDir: sinon.stub().yields(),
+      ensureDir: sinon.stub().resolves(),
     }
 
-    this.CompileManager = SandboxedModule.require(modulePath, {
+    this.CompileManager = SandboxedModule.require(MODULE_PATH, {
       requires: {
         './LatexRunner': this.LatexRunner,
         './ResourceWriter': this.ResourceWriter,
@@ -117,7 +127,7 @@ describe('CompileManager', function () {
         './TikzManager': this.TikzManager,
         './LockManager': this.LockManager,
         './SynctexOutputParser': this.SynctexOutputParser,
-        fs: this.fs,
+        'fs/promises': this.fsPromises,
         'fs-extra': this.fse,
       },
     })
@@ -141,45 +151,44 @@ describe('CompileManager', function () {
     })
 
     describe('when the project is locked', function () {
-      beforeEach(function () {
+      beforeEach(async function () {
-        this.error = new Error('locked')
+        const error = new Error('locked')
-        this.LockManager.runWithLock.callsFake((lockFile, runner, callback) => {
+        this.LockManager.acquire.rejects(error)
-          callback(this.error)
+        await expect(
-        })
+          this.CompileManager.promises.doCompileWithLock(this.request)
-        this.CompileManager.doCompileWithLock(this.request, this.callback)
+        ).to.be.rejectedWith(error)
       })
 
       it('should ensure that the compile directory exists', function () {
-        this.fse.ensureDir.calledWith(this.compileDir).should.equal(true)
+        expect(this.fse.ensureDir).to.have.been.calledWith(this.compileDir)
       })
 
       it('should not run LaTeX', function () {
-        this.LatexRunner.runLatex.called.should.equal(false)
+        expect(this.LatexRunner.promises.runLatex).not.to.have.been.called
-      })
-
-      it('should call the callback with the error', function () {
-        this.callback.calledWithExactly(this.error).should.equal(true)
       })
     })
 
     describe('normally', function () {
-      beforeEach(function () {
+      beforeEach(async function () {
-        this.CompileManager.doCompileWithLock(this.request, this.callback)
+        this.result = await this.CompileManager.promises.doCompileWithLock(
+          this.request
+        )
       })
 
       it('should ensure that the compile directory exists', function () {
-        this.fse.ensureDir.calledWith(this.compileDir).should.equal(true)
+        expect(this.fse.ensureDir).to.have.been.calledWith(this.compileDir)
       })
 
       it('should write the resources to disk', function () {
-        this.ResourceWriter.syncResourcesToDisk
+        expect(
-          .calledWith(this.request, this.compileDir)
+          this.ResourceWriter.promises.syncResourcesToDisk
-          .should.equal(true)
+        ).to.have.been.calledWith(this.request, this.compileDir)
       })
 
       it('should run LaTeX', function () {
-        this.LatexRunner.runLatex
+        expect(this.LatexRunner.promises.runLatex).to.have.been.calledWith(
-          .calledWith(`${this.projectId}-${this.userId}`, {
+          `${this.projectId}-${this.userId}`,
+          {
             directory: this.compileDir,
             mainFile: this.rootResourcePath,
             compiler: this.compiler,
@@ -189,47 +198,49 @@ describe('CompileManager', function () {
             environment: this.env,
             compileGroup: this.compileGroup,
             stopOnFirstError: this.request.stopOnFirstError,
-          })
+          }
-          .should.equal(true)
+        )
       })
 
       it('should find the output files', function () {
-        this.OutputFileFinder.findOutputFiles
+        expect(
-          .calledWith(this.resources, this.compileDir)
+          this.OutputFileFinder.promises.findOutputFiles
-          .should.equal(true)
+        ).to.have.been.calledWith(this.resources, this.compileDir)
       })
 
       it('should return the output files', function () {
-        this.callback.calledWith(null, this.buildFiles).should.equal(true)
+        expect(this.result.outputFiles).to.equal(this.buildFiles)
       })
 
       it('should not inject draft mode by default', function () {
-        this.DraftModeManager.injectDraftMode.called.should.equal(false)
+        expect(this.DraftModeManager.promises.injectDraftMode).not.to.have.been
+          .called
       })
     })
 
     describe('with draft mode', function () {
-      beforeEach(function () {
+      beforeEach(async function () {
         this.request.draft = true
-        this.CompileManager.doCompileWithLock(this.request, this.callback)
+        await this.CompileManager.promises.doCompileWithLock(this.request)
       })
 
       it('should inject the draft mode header', function () {
-        this.DraftModeManager.injectDraftMode
+        expect(
-          .calledWith(this.compileDir + '/' + this.rootResourcePath)
+          this.DraftModeManager.promises.injectDraftMode
-          .should.equal(true)
+        ).to.have.been.calledWith(this.compileDir + '/' + this.rootResourcePath)
       })
     })
 
     describe('with a check option', function () {
-      beforeEach(function () {
+      beforeEach(async function () {
         this.request.check = 'error'
-        this.CompileManager.doCompileWithLock(this.request, this.callback)
+        await this.CompileManager.promises.doCompileWithLock(this.request)
       })
 
       it('should run chktex', function () {
-        this.LatexRunner.runLatex
+        expect(this.LatexRunner.promises.runLatex).to.have.been.calledWith(
-          .calledWith(`${this.projectId}-${this.userId}`, {
+          `${this.projectId}-${this.userId}`,
+          {
             directory: this.compileDir,
             mainFile: this.rootResourcePath,
             compiler: this.compiler,
@@ -243,21 +254,22 @@ describe('CompileManager', function () {
             },
             compileGroup: this.compileGroup,
             stopOnFirstError: this.request.stopOnFirstError,
-          })
+          }
-          .should.equal(true)
+        )
       })
     })
 
     describe('with a knitr file and check options', function () {
-      beforeEach(function () {
+      beforeEach(async function () {
         this.request.rootResourcePath = 'main.Rtex'
         this.request.check = 'error'
-        this.CompileManager.doCompileWithLock(this.request, this.callback)
+        await this.CompileManager.promises.doCompileWithLock(this.request)
       })
 
       it('should not run chktex', function () {
-        this.LatexRunner.runLatex
+        expect(this.LatexRunner.promises.runLatex).to.have.been.calledWith(
-          .calledWith(`${this.projectId}-${this.userId}`, {
+          `${this.projectId}-${this.userId}`,
+          {
             directory: this.compileDir,
             mainFile: 'main.Rtex',
             compiler: this.compiler,
@@ -267,69 +279,58 @@ describe('CompileManager', function () {
             environment: this.env,
             compileGroup: this.compileGroup,
             stopOnFirstError: this.request.stopOnFirstError,
-          })
+          }
-          .should.equal(true)
+        )
       })
     })
   })
 
   describe('clearProject', function () {
     describe('succesfully', function () {
-      beforeEach(function () {
+      beforeEach(async function () {
         this.Settings.compileDir = 'compiles'
-        this.fs.lstat.yields(null, {
+        this.fsPromises.lstat.resolves({
           isDirectory() {
             return true
           },
         })
-        this.CompileManager.clearProject(
+        await this.CompileManager.promises.clearProject(
           this.projectId,
-          this.userId,
+          this.userId
-          this.callback
         )
-        this.proc.emit('close', 0)
       })
 
       it('should remove the project directory', function () {
-        this.child_process.spawn
+        expect(this.child_process.execFile).to.have.been.calledWith('rm', [
-          .calledWith('rm', ['-r', '-f', '--', this.compileDir])
+          '-r',
-          .should.equal(true)
+          '-f',
-      })
+          '--',
-
+          this.compileDir,
-      it('should call the callback', function () {
+        ])
-        this.callback.called.should.equal(true)
       })
     })
 
     describe('with a non-success status code', function () {
-      beforeEach(function () {
+      beforeEach(async function () {
         this.Settings.compileDir = 'compiles'
-        this.fs.lstat.yields(null, {
+        this.fsPromises.lstat.resolves({
           isDirectory() {
             return true
           },
         })
-        this.CompileManager.clearProject(
+        this.child_process.execFile.yields(new Error('oops'))
-          this.projectId,
+        await expect(
-          this.userId,
+          this.CompileManager.promises.clearProject(this.projectId, this.userId)
-          this.callback
+        ).to.be.rejected
-        )
-        this.proc.stderr.emit('data', (this.error = 'oops'))
-        this.proc.emit('close', 1)
       })
 
       it('should remove the project directory', function () {
-        this.child_process.spawn
+        expect(this.child_process.execFile).to.have.been.calledWith('rm', [
-          .calledWith('rm', ['-r', '-f', '--', this.compileDir])
+          '-r',
-          .should.equal(true)
+          '-f',
-      })
+          '--',
-
+          this.compileDir,
-      it('should call the callback with an error from the stderr', function () {
+        ])
-        this.callback.calledWithExactly(sinon.match(Error)).should.equal(true)
-
-        this.callback.args[0][0].message.should.equal(
-          `rm -r ${this.compileDir} failed: ${this.error}`
-        )
       })
     })
   })
@@ -348,7 +349,7 @@ describe('CompileManager', function () {
 
     describe('syncFromCode', function () {
       beforeEach(function () {
-        this.fs.stat.yields(null, {
+        this.fsPromises.stat.resolves({
           isFile() {
             return true
           },
@@ -357,63 +358,62 @@ describe('CompileManager', function () {
         this.SynctexOutputParser.parseViewOutput
           .withArgs(this.commandOutput)
           .returns(this.records)
-        this.CompileManager.syncFromCode(
-          this.projectId,
-          this.userId,
-          this.filename,
-          this.line,
-          this.column,
-          '',
-          this.callback
-        )
       })
 
-      it('should execute the synctex binary', function () {
+      describe('normal case', function () {
-        const outputFilePath = `${this.compileDir}/output.pdf`
+        beforeEach(async function () {
-        const inputFilePath = `${this.compileDir}/${this.filename}`
+          this.result = await this.CompileManager.promises.syncFromCode(
-        this.CommandRunner.run.should.have.been.calledWith(
-          `${this.projectId}-${this.userId}`,
-          [
-            'synctex',
-            'view',
-            '-i',
-            `${this.line}:${this.column}:${inputFilePath}`,
-            '-o',
-            outputFilePath,
-          ],
-          this.compileDir,
-          this.Settings.clsi.docker.image,
-          60000,
-          {}
-        )
-      })
-
-      it('should call the callback with the parsed output', function () {
-        this.callback.should.have.been.calledWith(
-          null,
-          sinon.match.array.deepEquals(this.records)
-        )
-      })
-
-      describe('with a custom imageName', function () {
-        const customImageName = 'foo/bar:tag-0'
-        beforeEach(function () {
-          this.CommandRunner.run.reset()
-          this.CompileManager.syncFromCode(
             this.projectId,
             this.userId,
             this.filename,
             this.line,
             this.column,
-            customImageName,
+            ''
-            this.callback
+          )
+        })
+
+        it('should execute the synctex binary', function () {
+          const outputFilePath = `${this.compileDir}/output.pdf`
+          const inputFilePath = `${this.compileDir}/${this.filename}`
+          expect(this.CommandRunner.promises.run).to.have.been.calledWith(
+            `${this.projectId}-${this.userId}`,
+            [
+              'synctex',
+              'view',
+              '-i',
+              `${this.line}:${this.column}:${inputFilePath}`,
+              '-o',
+              outputFilePath,
+            ],
+            this.compileDir,
+            this.Settings.clsi.docker.image,
+            60000,
+            {}
+          )
+        })
+
+        it('should return the parsed output', function () {
+          expect(this.result).to.deep.equal(this.records)
+        })
+      })
+
+      describe('with a custom imageName', function () {
+        const customImageName = 'foo/bar:tag-0'
+        beforeEach(async function () {
+          await this.CompileManager.promises.syncFromCode(
+            this.projectId,
+            this.userId,
+            this.filename,
+            this.line,
+            this.column,
+            customImageName
           )
         })
 
         it('should execute the synctex binary in a custom docker image', function () {
           const outputFilePath = `${this.compileDir}/output.pdf`
           const inputFilePath = `${this.compileDir}/${this.filename}`
-          this.CommandRunner.run.should.have.been.calledWith(
+          expect(this.CommandRunner.promises.run).to.have.been.calledWith(
             `${this.projectId}-${this.userId}`,
             [
               'synctex',
@@ -434,7 +434,7 @@ describe('CompileManager', function () {
 
     describe('syncFromPdf', function () {
       beforeEach(function () {
-        this.fs.stat.yields(null, {
+        this.fsPromises.stat.resolves({
           isFile() {
             return true
           },
@@ -443,93 +443,89 @@ describe('CompileManager', function () {
         this.SynctexOutputParser.parseEditOutput
           .withArgs(this.commandOutput, this.compileDir)
           .returns(this.records)
-        this.CompileManager.syncFromPdf(
-          this.projectId,
-          this.userId,
-          this.page,
-          this.h,
-          this.v,
-          '',
-          this.callback
-        )
       })
 
-      it('should execute the synctex binary', function () {
+      describe('normal case', function () {
-        const outputFilePath = `${this.compileDir}/output.pdf`
+        beforeEach(async function () {
-        this.CommandRunner.run.should.have.been.calledWith(
+          this.result = await this.CompileManager.promises.syncFromPdf(
-          `${this.projectId}-${this.userId}`,
-          [
-            'synctex',
-            'edit',
-            '-o',
-            `${this.page}:${this.h}:${this.v}:${outputFilePath}`,
-          ],
-          this.compileDir,
-          this.Settings.clsi.docker.image,
-          60000,
-          {}
-        )
-      })
-
-      it('should call the callback with the parsed output', function () {
-        this.callback.should.have.been.calledWith(
-          null,
-          sinon.match.array.deepEquals(this.records)
-        )
-      })
-
-      describe('with a custom imageName', function () {
-        const customImageName = 'foo/bar:tag-1'
-        beforeEach(function () {
-          this.CommandRunner.run.reset()
-          this.CompileManager.syncFromPdf(
             this.projectId,
             this.userId,
             this.page,
             this.h,
             this.v,
-            customImageName,
+            ''
-            this.callback
+          )
+        })
+
+        it('should execute the synctex binary', function () {
+          const outputFilePath = `${this.compileDir}/output.pdf`
+          expect(this.CommandRunner.promises.run).to.have.been.calledWith(
+            `${this.projectId}-${this.userId}`,
+            [
+              'synctex',
+              'edit',
+              '-o',
+              `${this.page}:${this.h}:${this.v}:${outputFilePath}`,
+            ],
+            this.compileDir,
+            this.Settings.clsi.docker.image,
+            60000,
+            {}
+          )
+        })
+
+        it('should return the parsed output', function () {
+          expect(this.result).to.deep.equal(this.records)
+        })
+      })
+
+      describe('with a custom imageName', function () {
+        const customImageName = 'foo/bar:tag-1'
+        beforeEach(async function () {
+          await this.CompileManager.promises.syncFromPdf(
+            this.projectId,
+            this.userId,
+            this.page,
+            this.h,
+            this.v,
+            customImageName
           )
         })
 
         it('should execute the synctex binary in a custom docker image', function () {
           const outputFilePath = `${this.compileDir}/output.pdf`
-          this.CommandRunner.run
+          expect(this.CommandRunner.promises.run).to.have.been.calledWith(
-            .calledWith(
+            `${this.projectId}-${this.userId}`,
-              `${this.projectId}-${this.userId}`,
+            [
-              [
+              'synctex',
-                'synctex',
+              'edit',
-                'edit',
+              '-o',
-                '-o',
+              `${this.page}:${this.h}:${this.v}:${outputFilePath}`,
-                `${this.page}:${this.h}:${this.v}:${outputFilePath}`,
+            ],
-              ],
+            this.compileDir,
-              this.compileDir,
+            customImageName,
-              customImageName,
+            60000,
-              60000,
+            {}
-              {}
+          )
-            )
-            .should.equal(true)
         })
       })
     })
   })
 
   describe('wordcount', function () {
-    beforeEach(function () {
+    beforeEach(async function () {
       this.stdout = 'Encoding: ascii\nWords in text: 2'
-      this.fs.readFile.yields(null, this.stdout)
+      this.fsPromises.readFile.resolves(this.stdout)
 
       this.timeout = 60 * 1000
       this.filename = 'main.tex'
       this.image = 'example.com/image'
 
-      this.CompileManager.wordcount(
+      this.result = await this.CompileManager.promises.wordcount(
         this.projectId,
         this.userId,
         this.filename,
-        this.image,
+        this.image
-        this.callback
       )
     })
 
@@ -543,33 +539,29 @@ describe('CompileManager', function () {
         `-out=${this.filePath}.wc`,
       ]
 
-      this.CommandRunner.run
+      expect(this.CommandRunner.promises.run).to.have.been.calledWith(
-        .calledWith(
+        `${this.projectId}-${this.userId}`,
-          `${this.projectId}-${this.userId}`,
+        this.command,
-          this.command,
+        this.compileDir,
-          this.compileDir,
+        this.image,
-          this.image,
+        this.timeout,
-          this.timeout,
+        {}
-          {}
+      )
-        )
-        .should.equal(true)
     })
 
-    it('should call the callback with the parsed output', function () {
+    it('should return the parsed output', function () {
-      this.callback
+      expect(this.result).to.deep.equal({
-        .calledWith(null, {
+        encode: 'ascii',
-          encode: 'ascii',
+        textWords: 2,
-          textWords: 2,
+        headWords: 0,
-          headWords: 0,
+        outside: 0,
-          outside: 0,
+        headers: 0,
-          headers: 0,
+        elements: 0,
-          elements: 0,
+        mathInline: 0,
-          mathInline: 0,
+        mathDisplay: 0,
-          mathDisplay: 0,
+        errors: 0,
-          errors: 0,
+        messages: '',
-          messages: '',
+      })
-        })
-        .should.equal(true)
     })
   })
 })

services/clsi/test/unit/js/LockManagerTests.js

@@ -1,88 +1,71 @@
-/* eslint-disable
+const { expect } = require('chai')
-    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
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const SandboxedModule = require('sandboxed-module')
 const sinon = require('sinon')
-const modulePath = require('path').join(
+const mockFs = require('mock-fs')
-  __dirname,
+const OError = require('@overleaf/o-error')
-  '../../../app/js/LockManager'
+const LockManager = require('../../../app/js/LockManager')
-)
-const Path = require('path')
 const Errors = require('../../../app/js/Errors')
 
-describe('DockerLockManager', function () {
+describe('LockManager', function () {
   beforeEach(function () {
-    this.LockManager = SandboxedModule.require(modulePath, {
+    this.lockFile = '/local/compile/directory/.project-lock'
-      requires: {
+    mockFs({
-        '@overleaf/settings': {},
+      '/local/compile/directory': {},
-        fs: {
-          lstat: sinon.stub().callsArgWith(1),
-          readdir: sinon.stub().callsArgWith(1),
-        },
-        lockfile: (this.Lockfile = {}),
-      },
     })
-    return (this.lockFile = '/local/compile/directory/.project-lock')
+    this.clock = sinon.useFakeTimers()
   })
 
-  return describe('runWithLock', function () {
+  afterEach(function () {
-    beforeEach(function () {
+    mockFs.restore()
-      this.runner = sinon.stub().callsArgWith(0, null, 'foo', 'bar')
+    this.clock.restore()
-      return (this.callback = sinon.stub())
+  })
+
+  describe('when the lock is available', function () {
+    it('the lock can be acquired', async function () {
+      await LockManager.acquire(this.lockFile)
     })
 
-    describe('normally', function () {
+    it('acquiring a lock in a nonexistent directory throws an error with debug info', async function () {
-      beforeEach(function () {
+      const err = await expect(
-        this.Lockfile.lock = sinon.stub().callsArgWith(2, null)
+        LockManager.acquire('/invalid/path/.project-lock')
-        this.Lockfile.unlock = sinon.stub().callsArgWith(1, null)
+      ).to.be.rejected
-        return this.LockManager.runWithLock(
+      const info = OError.getFullInfo(err)
-          this.lockFile,
+      expect(info).to.have.keys(['statLock', 'statDir', 'readdirDir'])
-          this.runner,
+      expect(info.statLock.code).to.equal('ENOENT')
-          this.callback
+      expect(info.statDir.code).to.equal('ENOENT')
-        )
+      expect(info.readdirDir.code).to.equal('ENOENT')
-      })
+    })
+  })
 
-      it('should run the compile', function () {
+  describe('after the lock is acquired', function () {
-        return this.runner.calledWith().should.equal(true)
+    beforeEach(async function () {
-      })
+      this.lock = await LockManager.acquire(this.lockFile)
-
-      return it('should call the callback with the response from the compile', function () {
-        return this.callback
-          .calledWithExactly(null, 'foo', 'bar')
-          .should.equal(true)
-      })
     })
 
-    return describe('when the project is locked', function () {
+    it("the lock can't be acquired again", function (done) {
-      beforeEach(function () {
+      const promise = LockManager.acquire(this.lockFile)
-        this.error = new Error()
+      // runAllAsync() will advance through time until there are no pending
-        this.error.code = 'EEXIST'
+      // timers or promises. It interferes with Mocha's promise interface, so
-        this.Lockfile.lock = sinon.stub().callsArgWith(2, this.error)
+      // we use Mocha's callback interface for this test.
-        this.Lockfile.unlock = sinon.stub().callsArgWith(1, null)
+      this.clock.runAllAsync()
-        return this.LockManager.runWithLock(
+      expect(promise)
-          this.lockFile,
+        .to.be.rejectedWith(Errors.AlreadyCompilingError)
-          this.runner,
+        .then(() => {
-          this.callback
+          done()
-        )
+        })
-      })
+        .catch(err => {
+          done(err)
+        })
+    })
 
-      it('should not run the compile', function () {
+    it('the lock can be acquired again after an expiry period', async function () {
-        return this.runner.called.should.equal(false)
+      // The expiry time is 5 minutes. Let's wait 10 minutes.
-      })
+      this.clock.tick(10 * 60 * 1000)
+      await LockManager.acquire(this.lockFile)
+    })
 
-      it('should return an error', function () {
+    it('the lock can be acquired again after it was released', async function () {
-        this.callback
+      this.lock.release()
-          .calledWithExactly(sinon.match(Errors.AlreadyCompilingError))
+      await LockManager.acquire(this.lockFile)
-          .should.equal(true)
-      })
     })
   })
 })