diff --git a/package-lock.json b/package-lock.json index 7e93e6e252..e851456814 100644 --- a/package-lock.json +++ b/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", diff --git a/services/clsi/app/js/CompileController.js b/services/clsi/app/js/CompileController.js index daa4e77f72..2c893226d8 100644 --- a/services/clsi/app/js/CompileController.js +++ b/services/clsi/app/js/CompileController.js @@ -29,93 +29,96 @@ function compile(req, res, next) { if (error) { return next(error) } - CompileManager.doCompileWithLock( - request, - function (error, outputFiles, stats, timings) { - let code, status - if (outputFiles == null) { - 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, - })), - }, - }) + CompileManager.doCompileWithLock(request, (error, result) => { + let { outputFiles, stats, timings } = result || {} + let code, status + if (outputFiles == null) { + 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' + 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, + })), + }, + }) + }) } ) }) diff --git a/services/clsi/app/js/CompileManager.js b/services/clsi/app/js/CompileManager.js index b9800bcc6b..e51052c85c 100644 --- a/services/clsi/app/js/CompileManager.js +++ b/services/clsi/app/js/CompileManager.js @@ -1,24 +1,29 @@ +const childProcess = require('child_process') +const fsPromises = require('fs/promises') +const fse = require('fs-extra') +const os = require('os') +const Path = require('path') +const { callbackify, promisify } = require('util') + +const Settings = require('@overleaf/settings') +const logger = require('@overleaf/logger') +const OError = require('@overleaf/o-error') + const ResourceWriter = require('./ResourceWriter') const LatexRunner = require('./LatexRunner') const OutputFileFinder = require('./OutputFileFinder') const OutputCacheManager = require('./OutputCacheManager') -const Settings = require('@overleaf/settings') -const Path = require('path') -const logger = require('@overleaf/logger') const Metrics = require('./Metrics') -const childProcess = require('child_process') const DraftModeManager = require('./DraftModeManager') const TikzManager = require('./TikzManager') const LockManager = require('./LockManager') -const fs = require('fs') -const fse = require('fs-extra') -const os = require('os') -const async = require('async') const Errors = require('./Errors') const CommandRunner = require('./CommandRunner') const { emitPdfStats } = require('./ContentCacheMetrics') const SynctexOutputParser = require('./SynctexOutputParser') +const execFile = promisify(childProcess.execFile) + const COMPILE_TIME_BUCKETS = [ // NOTE: These buckets are locked in per metric name. // If you want to change them, you will need to rename metrics. @@ -41,24 +46,21 @@ function getOutputDir(projectId, userId) { return Path.join(Settings.path.outputDir, getCompileName(projectId, userId)) } -function doCompileWithLock(request, callback) { +async function doCompileWithLock(request) { const compileDir = getCompileDir(request.project_id, request.user_id) - const lockFile = Path.join(compileDir, '.project-lock') // use a .project-lock file in the compile directory to prevent // simultaneous compiles - fse.ensureDir(compileDir, error => { - if (error) { - return callback(error) - } - LockManager.runWithLock( - lockFile, - releaseLock => doCompile(request, releaseLock), - callback - ) - }) + const lockFile = Path.join(compileDir, '.project-lock') + await fse.ensureDir(compileDir) + const lock = await LockManager.acquire(lockFile) + try { + return await doCompile(request) + } finally { + await lock.release() + } } -function doCompile(request, callback) { +async function doCompile(request) { const compileDir = getCompileDir(request.project_id, request.user_id) const outputDir = getOutputDir(request.project_id, request.user_id) @@ -68,395 +70,328 @@ function doCompile(request, callback) { request.metricsOpts, COMPILE_TIME_BUCKETS ) - const timer = new Metrics.Timer('write-to-disk', 1, request.metricsOpts) + const writeToDiskTimer = new Metrics.Timer( + 'write-to-disk', + 1, + request.metricsOpts + ) logger.debug( { projectId: request.project_id, userId: request.user_id }, 'syncing resources to disk' ) - ResourceWriter.syncResourcesToDisk( - request, - compileDir, - (error, resourceList) => { - // NOTE: resourceList is insecure, it should only be used to exclude files from the output list - if (error && error instanceof Errors.FilesOutOfSyncError) { - logger.warn( - { projectId: request.project_id, userId: request.user_id }, - 'files out of sync, please retry' - ) - return callback(error) - } else if (error) { - logger.err( - { - err: error, - projectId: request.project_id, - userId: request.user_id, - }, - 'error writing resources to disk' - ) - return callback(error) - } - logger.debug( - { - projectId: request.project_id, - userId: request.user_id, - time_taken: Date.now() - timer.start, - }, - 'written files to disk' - ) - const syncStage = timer.done() - function injectDraftModeIfRequired(callback) { - if (request.draft) { - DraftModeManager.injectDraftMode( - Path.join(compileDir, request.rootResourcePath), - callback - ) - } else { - callback() - } - } - - const createTikzFileIfRequired = callback => - TikzManager.checkMainFile( - compileDir, - request.rootResourcePath, - resourceList, - (error, needsMainFile) => { - if (error) { - return callback(error) - } - if (needsMainFile) { - TikzManager.injectOutputFile( - compileDir, - request.rootResourcePath, - callback - ) - } else { - callback() - } - } - ) - // set up environment variables for chktex - const env = {} - if (Settings.texliveOpenoutAny && Settings.texliveOpenoutAny !== '') { - // override default texlive openout_any environment variable - env.openout_any = Settings.texliveOpenoutAny - } - if (Settings.texliveMaxPrintLine && Settings.texliveMaxPrintLine !== '') { - // override default texlive max_print_line environment variable - env.max_print_line = Settings.texliveMaxPrintLine - } - // only run chktex on LaTeX files (not knitr .Rtex files or any others) - const isLaTeXFile = - request.rootResourcePath != null - ? request.rootResourcePath.match(/\.tex$/i) - : undefined - if (request.check != null && isLaTeXFile) { - env.CHKTEX_OPTIONS = '-nall -e9 -e10 -w15 -w16' - env.CHKTEX_ULIMIT_OPTIONS = '-t 5 -v 64000' - if (request.check === 'error') { - env.CHKTEX_EXIT_ON_ERROR = 1 - } - if (request.check === 'validate') { - env.CHKTEX_VALIDATE = 1 - } - } - - // apply a series of file modifications/creations for draft mode and tikz - async.series( - [injectDraftModeIfRequired, createTikzFileIfRequired], - error => { - if (error) { - return callback(error) - } - const timer = new Metrics.Timer('run-compile', 1, request.metricsOpts) - // find the image tag to log it as a metric, e.g. 2015.1 (convert . to - for graphite) - let tag = 'default' - if (request.imageName != null) { - const match = request.imageName.match(/:(.*)/) - if (match != null) { - tag = match[1].replace(/\./g, '-') - } - } - if (!request.project_id.match(/^[0-9a-f]{24}$/)) { - tag = 'other' - } // exclude smoke test - Metrics.inc('compiles', 1, request.metricsOpts) - Metrics.inc(`compiles-with-image.${tag}`, 1, request.metricsOpts) - const compileName = getCompileName( - request.project_id, - request.user_id - ) - LatexRunner.runLatex( - compileName, - { - directory: compileDir, - mainFile: request.rootResourcePath, - compiler: request.compiler, - timeout: request.timeout, - image: request.imageName, - flags: request.flags, - environment: env, - compileGroup: request.compileGroup, - stopOnFirstError: request.stopOnFirstError, - }, - (error, output, stats, timings) => { - // request was for validation only - if (request.check === 'validate') { - const result = error && error.code ? 'fail' : 'pass' - error = new Error('validation') - error.validate = result - } - // request was for compile, and failed on validation - if ( - request.check === 'error' && - error && - error.message === 'exited' - ) { - error = new Error('compilation') - error.validate = 'fail' - } - // record timeout errors as a separate counter, success is recorded later - if (error && error.timedout) { - Metrics.inc('compiles-timeout', 1, request.metricsOpts) - } - // compile was killed by user, was a validation, or a compile which failed validation - if ( - error && - (error.terminated || error.validate || error.timedout) - ) { - return OutputFileFinder.findOutputFiles( - resourceList, - compileDir, - (err, outputFiles) => { - if (err) { - return callback(err) - } - error.outputFiles = outputFiles // return output files so user can check logs - callback(error) - } - ) - } - // compile completed normally - if (error) { - return callback(error) - } - Metrics.inc('compiles-succeeded', 1, request.metricsOpts) - stats = stats || {} - for (const metricKey in stats) { - const metricValue = stats[metricKey] - Metrics.count(metricKey, metricValue, 1, request.metricsOpts) - } - timings = timings || {} - for (const metricKey in timings) { - const metricValue = timings[metricKey] - Metrics.timing(metricKey, metricValue, 1, request.metricsOpts) - } - const loadavg = - typeof os.loadavg === 'function' ? os.loadavg() : undefined - if (loadavg != null) { - Metrics.gauge('load-avg', loadavg[0]) - } - const ts = timer.done() - logger.debug( - { - projectId: request.project_id, - userId: request.user_id, - time_taken: ts, - stats, - timings, - loadavg, - }, - 'done compile' - ) - if (stats['latex-runs'] > 0) { - Metrics.histogram( - 'avg-compile-per-pass-v2', - ts / stats['latex-runs'], - COMPILE_TIME_BUCKETS, - request.metricsOpts - ) - Metrics.timing( - 'avg-compile-per-pass-v2', - ts / stats['latex-runs'], - 1, - request.metricsOpts - ) - } - if (stats['latex-runs'] > 0 && timings['cpu-time'] > 0) { - Metrics.timing( - 'run-compile-cpu-time-per-pass', - timings['cpu-time'] / stats['latex-runs'], - 1, - request.metricsOpts - ) - } - // Emit compile time. - timings.compile = ts - - const outputStageTimer = new Metrics.Timer( - 'process-output-files', - 1, - request.metricsOpts - ) - - OutputFileFinder.findOutputFiles( - resourceList, - compileDir, - (error, outputFiles) => { - if (error) { - return callback(error) - } - OutputCacheManager.saveOutputFiles( - { request, stats, timings }, - outputFiles, - compileDir, - outputDir, - (err, newOutputFiles) => { - if (err) { - const { project_id: projectId, user_id: userId } = - request - logger.err( - { projectId, userId, err }, - 'failed to save output files' - ) - } - - const outputStage = outputStageTimer.done() - timings.sync = syncStage - timings.output = outputStage - - // Emit e2e compile time. - timings.compileE2E = timerE2E.done() - Metrics.timing( - 'compile-e2e-v2', - timings.compileE2E, - 1, - request.metricsOpts - ) - - if (stats['pdf-size']) { - emitPdfStats(stats, timings, request) - } - - callback(null, newOutputFiles, stats, timings) - } - ) - } - ) - } - ) - } - ) + let resourceList + try { + // NOTE: resourceList is insecure, it should only be used to exclude files from the output list + resourceList = await ResourceWriter.promises.syncResourcesToDisk( + request, + compileDir + ) + } catch (error) { + if (error instanceof Errors.FilesOutOfSyncError) { + OError.tag(error, 'files out of sync, please retry', { + projectId: request.project_id, + userId: request.user_id, + }) + } else { + OError.tag(error, 'error writing resources to disk', { + projectId: request.project_id, + userId: request.user_id, + }) } + throw error + } + logger.debug( + { + projectId: request.project_id, + userId: request.user_id, + time_taken: Date.now() - writeToDiskTimer.start, + }, + 'written files to disk' ) -} + const syncStage = writeToDiskTimer.done() -function stopCompile(projectId, userId, callback) { - const compileName = getCompileName(projectId, userId) - LatexRunner.killLatex(compileName, callback) -} - -function clearProject(projectId, userId, _callback) { - function callback(error) { - _callback(error) - _callback = function () {} + // set up environment variables for chktex + const env = {} + if (Settings.texliveOpenoutAny && Settings.texliveOpenoutAny !== '') { + // override default texlive openout_any environment variable + env.openout_any = Settings.texliveOpenoutAny + } + if (Settings.texliveMaxPrintLine && Settings.texliveMaxPrintLine !== '') { + // override default texlive max_print_line environment variable + env.max_print_line = Settings.texliveMaxPrintLine + } + // only run chktex on LaTeX files (not knitr .Rtex files or any others) + const isLaTeXFile = request.rootResourcePath?.match(/\.tex$/i) + if (request.check != null && isLaTeXFile) { + env.CHKTEX_OPTIONS = '-nall -e9 -e10 -w15 -w16' + env.CHKTEX_ULIMIT_OPTIONS = '-t 5 -v 64000' + if (request.check === 'error') { + env.CHKTEX_EXIT_ON_ERROR = 1 + } + if (request.check === 'validate') { + env.CHKTEX_VALIDATE = 1 + } } + // apply a series of file modifications/creations for draft mode and tikz + if (request.draft) { + await DraftModeManager.promises.injectDraftMode( + Path.join(compileDir, request.rootResourcePath) + ) + } + + const needsMainFile = await TikzManager.promises.checkMainFile( + compileDir, + request.rootResourcePath, + resourceList + ) + if (needsMainFile) { + await TikzManager.promises.injectOutputFile( + compileDir, + request.rootResourcePath + ) + } + + const compileTimer = new Metrics.Timer('run-compile', 1, request.metricsOpts) + // find the image tag to log it as a metric, e.g. 2015.1 (convert . to - for graphite) + let tag = 'default' + if (request.imageName != null) { + const match = request.imageName.match(/:(.*)/) + if (match != null) { + tag = match[1].replace(/\./g, '-') + } + } + // exclude smoke test + if (!request.project_id.match(/^[0-9a-f]{24}$/)) { + tag = 'other' + } + Metrics.inc('compiles', 1, request.metricsOpts) + Metrics.inc(`compiles-with-image.${tag}`, 1, request.metricsOpts) + const compileName = getCompileName(request.project_id, request.user_id) + + let compileResult + try { + compileResult = await LatexRunner.promises.runLatex(compileName, { + directory: compileDir, + mainFile: request.rootResourcePath, + compiler: request.compiler, + timeout: request.timeout, + image: request.imageName, + flags: request.flags, + environment: env, + compileGroup: request.compileGroup, + stopOnFirstError: request.stopOnFirstError, + }) + + // We use errors to return the validation state. It would be nice to use a + // more appropriate mechanism. + if (request.check === 'validate') { + const validationError = new Error('validation') + validationError.validate = 'pass' + throw validationError + } + } catch (originalError) { + let error = originalError + // request was for validation only + if (request.check === 'validate' && !error.validate) { + error = new Error('validation') + error.validate = originalError.code ? 'fail' : 'pass' + } + + // request was for compile, and failed on validation + if (request.check === 'error' && originalError.message === 'exited') { + error = new Error('compilation') + error.validate = 'fail' + } + + // compile was killed by user, was a validation, or a compile which failed validation + if (error.terminated || error.validate || error.timedout) { + // record timeout errors as a separate counter, success is recorded later + if (error.timedout) { + Metrics.inc('compiles-timeout', 1, request.metricsOpts) + } + + const { outputFiles } = await OutputFileFinder.promises.findOutputFiles( + resourceList, + compileDir + ) + error.outputFiles = outputFiles // return output files so user can check logs + } + throw error + } + + // compile completed normally + let { stats, timings } = compileResult + stats = stats || {} + timings = timings || {} + Metrics.inc('compiles-succeeded', 1, request.metricsOpts) + for (const metricKey in stats) { + const metricValue = stats[metricKey] + Metrics.count(metricKey, metricValue, 1, request.metricsOpts) + } + for (const metricKey in timings) { + const metricValue = timings[metricKey] + Metrics.timing(metricKey, metricValue, 1, request.metricsOpts) + } + const loadavg = typeof os.loadavg === 'function' ? os.loadavg() : undefined + if (loadavg != null) { + Metrics.gauge('load-avg', loadavg[0]) + } + const ts = compileTimer.done() + logger.debug( + { + projectId: request.project_id, + userId: request.user_id, + time_taken: ts, + stats, + timings, + loadavg, + }, + 'done compile' + ) + if (stats['latex-runs'] > 0) { + Metrics.histogram( + 'avg-compile-per-pass-v2', + ts / stats['latex-runs'], + COMPILE_TIME_BUCKETS, + request.metricsOpts + ) + Metrics.timing( + 'avg-compile-per-pass-v2', + ts / stats['latex-runs'], + 1, + request.metricsOpts + ) + } + if (stats['latex-runs'] > 0 && timings['cpu-time'] > 0) { + Metrics.timing( + 'run-compile-cpu-time-per-pass', + timings['cpu-time'] / stats['latex-runs'], + 1, + request.metricsOpts + ) + } + // Emit compile time. + timings.compile = ts + + const outputStageTimer = new Metrics.Timer( + 'process-output-files', + 1, + request.metricsOpts + ) + + let { outputFiles } = await OutputFileFinder.promises.findOutputFiles( + resourceList, + compileDir + ) + + try { + outputFiles = await OutputCacheManager.promises.saveOutputFiles( + { request, stats, timings }, + outputFiles, + compileDir, + outputDir + ) + } catch (err) { + const { project_id: projectId, user_id: userId } = request + logger.err({ projectId, userId, err }, 'failed to save output files') + } + + const outputStage = outputStageTimer.done() + timings.sync = syncStage + timings.output = outputStage + + // Emit e2e compile time. + timings.compileE2E = timerE2E.done() + Metrics.timing('compile-e2e-v2', timings.compileE2E, 1, request.metricsOpts) + + if (stats['pdf-size']) { + emitPdfStats(stats, timings, request) + } + + return { outputFiles, stats, timings } +} + +async function stopCompile(projectId, userId) { + const compileName = getCompileName(projectId, userId) + await LatexRunner.promises.killLatex(compileName) +} + +async function clearProject(projectId, userId) { const compileDir = getCompileDir(projectId, userId) - _checkDirectory(compileDir, (err, exists) => { - if (err) { - return callback(err) - } - if (!exists) { - return callback() - } // skip removal if no directory present + const exists = await _checkDirectory(compileDir) + if (!exists) { + // skip removal if no directory present + return + } - const proc = childProcess.spawn('rm', ['-r', '-f', '--', compileDir]) - - proc.on('error', callback) - - let stderr = '' - proc.stderr.setEncoding('utf8').on('data', chunk => (stderr += chunk)) - - proc.on('close', code => { - if (code === 0) { - callback(null) - } else { - callback(new Error(`rm -r ${compileDir} failed: ${stderr}`)) - } - }) - }) + try { + await execFile('rm', ['-r', '-f', '--', compileDir]) + } catch (err) { + OError.tag(err, `rm -r failed`, { compileDir, stderr: err.stderr }) + throw err + } } -function _findAllDirs(callback) { +async function _findAllDirs() { const root = Settings.path.compilesDir - fs.readdir(root, (err, files) => { - if (err) { - return callback(err) - } - const allDirs = files.map(file => Path.join(root, file)) - callback(null, allDirs) - }) + const files = await fsPromises.readdir(root) + const allDirs = files.map(file => Path.join(root, file)) + return allDirs } -function clearExpiredProjects(maxCacheAgeMs, callback) { +async function clearExpiredProjects(maxCacheAgeMs) { const now = Date.now() - // action for each directory - const expireIfNeeded = (checkDir, cb) => - fs.stat(checkDir, (err, stats) => { - if (err) { - return cb() - } // ignore errors checking directory - const age = now - stats.mtime - const hasExpired = age > maxCacheAgeMs - if (hasExpired) { - fse.remove(checkDir, cb) - } else { - cb() - } + const dirs = await _findAllDirs() + for (const dir of dirs) { + let stats + try { + stats = await fsPromises.stat(dir) + } catch (err) { + // ignore errors checking directory + continue + } + + const age = now - stats.mtime + const hasExpired = age > maxCacheAgeMs + if (hasExpired) { + await fse.remove(dir) + } + } +} + +async function _checkDirectory(compileDir) { + let stats + try { + stats = await fsPromises.lstat(compileDir) + } catch (err) { + if (err.code === 'ENOENT') { + // directory does not exist + return false + } + OError.tag(err, 'error on stat of project directory for removal', { + dir: compileDir, }) - // iterate over all project directories - _findAllDirs((error, allDirs) => { - if (error) { - return callback() - } - async.eachSeries(allDirs, expireIfNeeded, callback) - }) + throw err + } + if (!stats.isDirectory()) { + throw new OError('project directory is not directory', { + dir: compileDir, + stats, + }) + } + return true } -function _checkDirectory(compileDir, callback) { - fs.lstat(compileDir, (err, stats) => { - if (err && err.code === 'ENOENT') { - callback(null, false) // directory does not exist - } else if (err) { - logger.err( - { dir: compileDir, err }, - 'error on stat of project directory for removal' - ) - callback(err) - } else if (!stats.isDirectory()) { - logger.err( - { dir: compileDir, stats }, - 'bad project directory for removal' - ) - callback(new Error('project directory is not directory')) - } else { - // directory exists - callback(null, true) - } - }) -} - -function syncFromCode( +async function syncFromCode( projectId, userId, filename, line, column, - imageName, - callback + imageName ) { // If LaTeX was run in a virtual environment, the file path that synctex expects // might not match the file path on the host. The .synctex.gz file however, will be accessed @@ -473,19 +408,15 @@ function syncFromCode( '-o', outputFilePath, ] - _runSynctex(projectId, userId, command, imageName, (error, stdout) => { - if (error) { - return callback(error) - } - logger.debug( - { projectId, userId, filename, line, column, command, stdout }, - 'synctex code output' - ) - callback(null, SynctexOutputParser.parseViewOutput(stdout)) - }) + const stdout = await _runSynctex(projectId, userId, command, imageName) + logger.debug( + { projectId, userId, filename, line, column, command, stdout }, + 'synctex code output' + ) + return SynctexOutputParser.parseViewOutput(stdout) } -function syncFromPdf(projectId, userId, page, h, v, imageName, callback) { +async function syncFromPdf(projectId, userId, page, h, v, imageName) { const compileName = getCompileName(projectId, userId) const baseDir = Settings.path.synctexBaseDir(compileName) const outputFilePath = `${baseDir}/output.pdf` @@ -495,76 +426,64 @@ function syncFromPdf(projectId, userId, page, h, v, imageName, callback) { '-o', `${page}:${h}:${v}:${outputFilePath}`, ] - _runSynctex(projectId, userId, command, imageName, (error, stdout) => { - if (error != null) { - return callback(error) - } - logger.debug( - { projectId, userId, page, h, v, stdout }, - 'synctex pdf output' - ) - callback(null, SynctexOutputParser.parseEditOutput(stdout, baseDir)) - }) + const stdout = await _runSynctex(projectId, userId, command, imageName) + logger.debug({ projectId, userId, page, h, v, stdout }, 'synctex pdf output') + return SynctexOutputParser.parseEditOutput(stdout, baseDir) } -function _checkFileExists(dir, filename, callback) { +async function _checkFileExists(dir, filename) { + try { + await fsPromises.stat(dir) + } catch (error) { + if (error.code === 'ENOENT') { + throw new Errors.NotFoundError('no output directory') + } + throw error + } + const file = Path.join(dir, filename) - fs.stat(dir, (error, stats) => { - if (error && error.code === 'ENOENT') { - return callback(new Errors.NotFoundError('no output directory')) + let stats + try { + stats = await fsPromises.stat(file) + } catch (error) { + if (error.code === 'ENOENT') { + throw new Errors.NotFoundError('no output file') } - if (error) { - return callback(error) - } - fs.stat(file, (error, stats) => { - if (error && error.code === 'ENOENT') { - return callback(new Errors.NotFoundError('no output file')) - } - if (error) { - return callback(error) - } - if (!stats.isFile()) { - return callback(new Error('not a file')) - } - callback() - }) - }) + } + if (!stats.isFile()) { + throw new Error('not a file') + } } -function _runSynctex(projectId, userId, command, imageName, callback) { +async function _runSynctex(projectId, userId, command, imageName) { const directory = getCompileDir(projectId, userId) const timeout = 60 * 1000 // increased to allow for large projects const compileName = getCompileName(projectId, userId) const compileGroup = 'synctex' const defaultImageName = Settings.clsi && Settings.clsi.docker && Settings.clsi.docker.image - _checkFileExists(directory, 'output.synctex.gz', error => { - if (error) { - return callback(error) - } - CommandRunner.run( + await _checkFileExists(directory, 'output.synctex.gz') + try { + const output = await CommandRunner.promises.run( compileName, command, directory, imageName || defaultImageName, timeout, {}, - compileGroup, - (error, output) => { - if (error) { - logger.err( - { err: error, command, projectId, userId }, - 'error running synctex' - ) - return callback(error) - } - callback(null, output.stdout) - } + compileGroup ) - }) + return output.stdout + } catch (error) { + throw OError.tag(error, 'error running synctex', { + command, + projectId, + userId, + }) + } } -function wordcount(projectId, userId, filename, image, callback) { +async function wordcount(projectId, userId, filename, image) { logger.debug({ projectId, userId, filename, image }, 'running wordcount') const filePath = `$COMPILE_DIR/${filename}` const command = [ @@ -578,49 +497,43 @@ function wordcount(projectId, userId, filename, image, callback) { const timeout = 60 * 1000 const compileName = getCompileName(projectId, userId) const compileGroup = 'wordcount' - fse.ensureDir(compileDir, error => { - if (error) { - logger.err( - { error, projectId, userId, filename }, - 'error ensuring dir for sync from code' - ) - return callback(error) - } - CommandRunner.run( - compileName, + try { + await fse.ensureDir(compileDir) + } catch (err) { + throw OError.tag(err, 'error ensuring dir for wordcount', { + projectId, + userId, + filename, + }) + } + await CommandRunner.promises.run( + compileName, + command, + compileDir, + image, + timeout, + {}, + compileGroup + ) + + let stdout + try { + stdout = await fsPromises.readFile( + compileDir + '/' + filename + '.wc', + 'utf-8' + ) + } catch (err) { + throw OError.tag(err, 'error reading word count output', { command, compileDir, - image, - timeout, - {}, - compileGroup, - error => { - if (error) { - return callback(error) - } - fs.readFile( - compileDir + '/' + filename + '.wc', - 'utf-8', - (err, stdout) => { - if (err) { - // call it node_err so sentry doesn't use random path error as unique id so it can't be ignored - logger.err( - { node_err: err, command, compileDir, projectId, userId }, - 'error reading word count output' - ) - return callback(err) - } - const results = _parseWordcountFromOutput(stdout) - logger.debug( - { projectId, userId, wordcount: results }, - 'word count results' - ) - callback(null, results) - } - ) - } - ) - }) + projectId, + userId, + }) + } + + const results = _parseWordcountFromOutput(stdout) + logger.debug({ projectId, userId, wordcount: results }, 'word count results') + return results } function _parseWordcountFromOutput(output) { @@ -675,11 +588,20 @@ function _parseWordcountFromOutput(output) { } module.exports = { - doCompileWithLock, - stopCompile, - clearProject, - clearExpiredProjects, - syncFromCode, - syncFromPdf, - wordcount, + doCompileWithLock: callbackify(doCompileWithLock), + stopCompile: callbackify(stopCompile), + clearProject: callbackify(clearProject), + clearExpiredProjects: callbackify(clearExpiredProjects), + syncFromCode: callbackify(syncFromCode), + syncFromPdf: callbackify(syncFromPdf), + wordcount: callbackify(wordcount), + promises: { + doCompileWithLock, + stopCompile, + clearProject, + clearExpiredProjects, + syncFromCode, + syncFromPdf, + wordcount, + }, } diff --git a/services/clsi/app/js/DockerRunner.js b/services/clsi/app/js/DockerRunner.js index f699ebf16e..3ebabe25d2 100644 --- a/services/clsi/app/js/DockerRunner.js +++ b/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), +} diff --git a/services/clsi/app/js/DraftModeManager.js b/services/clsi/app/js/DraftModeManager.js index a3b078fc0f..743c4d1445 100644 --- a/services/clsi/app/js/DraftModeManager.js +++ b/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), +} diff --git a/services/clsi/app/js/LatexRunner.js b/services/clsi/app/js/LatexRunner.js index cfc385e4b9..bc877809c5 100644 --- a/services/clsi/app/js/LatexRunner.js +++ b/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), + }, } diff --git a/services/clsi/app/js/LocalCommandRunner.js b/services/clsi/app/js/LocalCommandRunner.js index 8b3180ac48..9f86b26e9d 100644 --- a/services/clsi/app/js/LocalCommandRunner.js +++ b/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), +} diff --git a/services/clsi/app/js/LockManager.js b/services/clsi/app/js/LockManager.js index d53872c342..f73138dcb2 100644 --- a/services/clsi/app/js/LockManager.js +++ b/services/clsi/app/js/LockManager.js @@ -1,71 +1,61 @@ -/* eslint-disable - 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 - * 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 { promisify } = require('util') +const OError = require('@overleaf/o-error') +const Lockfile = require('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) { - if (callback == null) { - callback = function () {} - } - 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 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 } + +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 } diff --git a/services/clsi/app/js/OutputCacheManager.js b/services/clsi/app/js/OutputCacheManager.js index fa9f0050a6..bb270bfa31 100644 --- a/services/clsi/app/js/OutputCacheManager.js +++ b/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 ), diff --git a/services/clsi/app/js/OutputFileFinder.js b/services/clsi/app/js/OutputFileFinder.js index c34945fcba..2ecd78845d 100644 --- a/services/clsi/app/js/OutputFileFinder.js +++ b/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 }) + } + } + ) + }), +} diff --git a/services/clsi/app/js/ResourceWriter.js b/services/clsi/app/js/ResourceWriter.js index 6c897f8d8d..da610508b8 100644 --- a/services/clsi/app/js/ResourceWriter.js +++ b/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) { - if (error != null) { - return callback(error) - } - return ResourceStateManager.saveProjectState( - request.syncState, - request.resources, - basePath, - function (error) { - if (error != null) { - return callback(error) - } - return callback(null, request.resources) + ResourceWriter.saveAllResourcesToDisk( + request, + basePath, + function (error) { + if (error != null) { + return callback(error) } - ) - }) + 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 => { - if (error != null) { - return callback(error) + ResourceWriter._removeExtraneousFiles( + request, + 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), +} diff --git a/services/clsi/app/js/TikzManager.js b/services/clsi/app/js/TikzManager.js index 505f1776ba..7d5f6c1b81 100644 --- a/services/clsi/app/js/TikzManager.js +++ b/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), +} diff --git a/services/clsi/package.json b/services/clsi/package.json index 0835bf8a3a..df79ebd1da 100644 --- a/services/clsi/package.json +++ b/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", diff --git a/services/clsi/test/setup.js b/services/clsi/test/setup.js index a889532022..653a8a69b2 100644 --- a/services/clsi/test/setup.js +++ b/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({ diff --git a/services/clsi/test/unit/js/CompileControllerTests.js b/services/clsi/test/unit/js/CompileControllerTests.js index e8a583cf8e..2b48aabbf0 100644 --- a/services/clsi/test/unit/js/CompileControllerTests.js +++ b/services/clsi/test/unit/js/CompileControllerTests.js @@ -111,9 +111,11 @@ describe('CompileController', function () { describe('successfully', function () { beforeEach(function () { - this.CompileManager.doCompileWithLock = sinon - .stub() - .yields(null, this.output_files, this.stats, this.timings) + this.CompileManager.doCompileWithLock = sinon.stub().yields(null, { + outputFiles: this.output_files, + 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 - .stub() - .yields(null, this.output_files, this.stats, this.timings) + this.CompileManager.doCompileWithLock = sinon.stub().yields(null, { + outputFiles: this.output_files, + 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 - .stub() - .yields(null, this.output_files, this.stats, this.timings) + this.CompileManager.doCompileWithLock = sinon.stub().yields(null, { + outputFiles: this.output_files, + 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 - .stub() - .yields(null, this.output_files, this.stats, this.timings) + this.CompileManager.doCompileWithLock = sinon.stub().yields(null, { + outputFiles: this.output_files, + stats: this.stats, + timings: this.timings, + }) this.CompileController.compile(this.req, this.res) }) diff --git a/services/clsi/test/unit/js/CompileManagerTests.js b/services/clsi/test/unit/js/CompileManagerTests.js index 237e938a58..128222b0b1 100644 --- a/services/clsi/test/unit/js/CompileManagerTests.js +++ b/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) => { - runner((err, ...result) => callback(err, ...result)) - }), + acquire: sinon.stub().resolves(this.lock), } 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 () { - this.error = new Error('locked') - this.LockManager.runWithLock.callsFake((lockFile, runner, callback) => { - callback(this.error) - }) - this.CompileManager.doCompileWithLock(this.request, this.callback) + beforeEach(async function () { + const error = new Error('locked') + this.LockManager.acquire.rejects(error) + await expect( + this.CompileManager.promises.doCompileWithLock(this.request) + ).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) - }) - - it('should call the callback with the error', function () { - this.callback.calledWithExactly(this.error).should.equal(true) + expect(this.LatexRunner.promises.runLatex).not.to.have.been.called }) }) describe('normally', function () { - beforeEach(function () { - this.CompileManager.doCompileWithLock(this.request, this.callback) + beforeEach(async function () { + 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 - .calledWith(this.request, this.compileDir) - .should.equal(true) + expect( + this.ResourceWriter.promises.syncResourcesToDisk + ).to.have.been.calledWith(this.request, this.compileDir) }) it('should run LaTeX', function () { - this.LatexRunner.runLatex - .calledWith(`${this.projectId}-${this.userId}`, { + expect(this.LatexRunner.promises.runLatex).to.have.been.calledWith( + `${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 - .calledWith(this.resources, this.compileDir) - .should.equal(true) + expect( + this.OutputFileFinder.promises.findOutputFiles + ).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 - .calledWith(this.compileDir + '/' + this.rootResourcePath) - .should.equal(true) + expect( + this.DraftModeManager.promises.injectDraftMode + ).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 - .calledWith(`${this.projectId}-${this.userId}`, { + expect(this.LatexRunner.promises.runLatex).to.have.been.calledWith( + `${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 - .calledWith(`${this.projectId}-${this.userId}`, { + expect(this.LatexRunner.promises.runLatex).to.have.been.calledWith( + `${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.callback + this.userId ) - this.proc.emit('close', 0) }) it('should remove the project directory', function () { - this.child_process.spawn - .calledWith('rm', ['-r', '-f', '--', this.compileDir]) - .should.equal(true) - }) - - it('should call the callback', function () { - this.callback.called.should.equal(true) + expect(this.child_process.execFile).to.have.been.calledWith('rm', [ + '-r', + '-f', + '--', + this.compileDir, + ]) }) }) 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.projectId, - this.userId, - this.callback - ) - this.proc.stderr.emit('data', (this.error = 'oops')) - this.proc.emit('close', 1) + this.child_process.execFile.yields(new Error('oops')) + await expect( + this.CompileManager.promises.clearProject(this.projectId, this.userId) + ).to.be.rejected }) it('should remove the project directory', function () { - this.child_process.spawn - .calledWith('rm', ['-r', '-f', '--', this.compileDir]) - .should.equal(true) - }) - - 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}` - ) + expect(this.child_process.execFile).to.have.been.calledWith('rm', [ + '-r', + '-f', + '--', + this.compileDir, + ]) }) }) }) @@ -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 () { - const outputFilePath = `${this.compileDir}/output.pdf` - const inputFilePath = `${this.compileDir}/${this.filename}` - 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( + describe('normal case', function () { + beforeEach(async function () { + this.result = await this.CompileManager.promises.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 () { - const outputFilePath = `${this.compileDir}/output.pdf` - this.CommandRunner.run.should.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 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( + describe('normal case', function () { + beforeEach(async function () { + this.result = await this.CompileManager.promises.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 - .calledWith( - `${this.projectId}-${this.userId}`, - [ - 'synctex', - 'edit', - '-o', - `${this.page}:${this.h}:${this.v}:${outputFilePath}`, - ], - this.compileDir, - customImageName, - 60000, - {} - ) - .should.equal(true) + 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, + customImageName, + 60000, + {} + ) }) }) }) }) 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.callback + this.image ) }) @@ -543,33 +539,29 @@ describe('CompileManager', function () { `-out=${this.filePath}.wc`, ] - this.CommandRunner.run - .calledWith( - `${this.projectId}-${this.userId}`, - this.command, - this.compileDir, - this.image, - this.timeout, - {} - ) - .should.equal(true) + expect(this.CommandRunner.promises.run).to.have.been.calledWith( + `${this.projectId}-${this.userId}`, + this.command, + this.compileDir, + this.image, + this.timeout, + {} + ) }) - it('should call the callback with the parsed output', function () { - this.callback - .calledWith(null, { - encode: 'ascii', - textWords: 2, - headWords: 0, - outside: 0, - headers: 0, - elements: 0, - mathInline: 0, - mathDisplay: 0, - errors: 0, - messages: '', - }) - .should.equal(true) + it('should return the parsed output', function () { + expect(this.result).to.deep.equal({ + encode: 'ascii', + textWords: 2, + headWords: 0, + outside: 0, + headers: 0, + elements: 0, + mathInline: 0, + mathDisplay: 0, + errors: 0, + messages: '', + }) }) }) }) diff --git a/services/clsi/test/unit/js/LockManagerTests.js b/services/clsi/test/unit/js/LockManagerTests.js index e109054801..048387067c 100644 --- a/services/clsi/test/unit/js/LockManagerTests.js +++ b/services/clsi/test/unit/js/LockManagerTests.js @@ -1,88 +1,71 @@ -/* 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 - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const SandboxedModule = require('sandboxed-module') +const { expect } = require('chai') const sinon = require('sinon') -const modulePath = require('path').join( - __dirname, - '../../../app/js/LockManager' -) -const Path = require('path') +const mockFs = require('mock-fs') +const OError = require('@overleaf/o-error') +const LockManager = require('../../../app/js/LockManager') const Errors = require('../../../app/js/Errors') -describe('DockerLockManager', function () { +describe('LockManager', function () { beforeEach(function () { - this.LockManager = SandboxedModule.require(modulePath, { - requires: { - '@overleaf/settings': {}, - fs: { - lstat: sinon.stub().callsArgWith(1), - readdir: sinon.stub().callsArgWith(1), - }, - lockfile: (this.Lockfile = {}), - }, + this.lockFile = '/local/compile/directory/.project-lock' + mockFs({ + '/local/compile/directory': {}, }) - return (this.lockFile = '/local/compile/directory/.project-lock') + this.clock = sinon.useFakeTimers() }) - return describe('runWithLock', function () { - beforeEach(function () { - this.runner = sinon.stub().callsArgWith(0, null, 'foo', 'bar') - return (this.callback = sinon.stub()) + 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) }) - describe('normally', function () { - beforeEach(function () { - this.Lockfile.lock = sinon.stub().callsArgWith(2, null) - this.Lockfile.unlock = sinon.stub().callsArgWith(1, null) - return this.LockManager.runWithLock( - this.lockFile, - this.runner, - this.callback - ) - }) + 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('should run the compile', function () { - return this.runner.calledWith().should.equal(true) - }) - - return it('should call the callback with the response from the compile', function () { - return this.callback - .calledWithExactly(null, 'foo', 'bar') - .should.equal(true) - }) + describe('after the lock is acquired', function () { + beforeEach(async function () { + this.lock = await LockManager.acquire(this.lockFile) }) - return describe('when the project is locked', function () { - beforeEach(function () { - this.error = new Error() - this.error.code = 'EEXIST' - this.Lockfile.lock = sinon.stub().callsArgWith(2, this.error) - this.Lockfile.unlock = sinon.stub().callsArgWith(1, null) - return this.LockManager.runWithLock( - this.lockFile, - this.runner, - this.callback - ) - }) + 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('should not run the compile', function () { - return this.runner.called.should.equal(false) - }) + 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('should return an error', function () { - this.callback - .calledWithExactly(sinon.match(Errors.AlreadyCompilingError)) - .should.equal(true) - }) + it('the lock can be acquired again after it was released', async function () { + this.lock.release() + await LockManager.acquire(this.lockFile) }) }) })