diff --git a/services/filestore/app/js/Errors.js b/services/filestore/app/js/Errors.js index f3bc8e37eb..4231571cb3 100644 --- a/services/filestore/app/js/Errors.js +++ b/services/filestore/app/js/Errors.js @@ -1,16 +1,38 @@ -/* eslint-disable - no-proto, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -let Errors -var NotFoundError = function(message) { - const error = new Error(message) - error.name = 'NotFoundError' - error.__proto__ = NotFoundError.prototype - return error -} -NotFoundError.prototype.__proto__ = Error.prototype +const OError = require('@overleaf/o-error') -module.exports = Errors = { NotFoundError } +// Error class for legacy errors so they inherit OError while staying +// backward-compatible (can be instantiated with string as argument instead +// of object) +class BackwardCompatibleError extends OError { + constructor(messageOrOptions) { + let options + if (typeof messageOrOptions === 'string') { + options = { message: messageOrOptions } + } else if (!messageOrOptions) { + options = {} + } else { + options = messageOrOptions + } + super(options) + } +} + +class NotFoundError extends BackwardCompatibleError {} +class ConversionsDisabledError extends BackwardCompatibleError {} + +class FailedCommandError extends OError { + constructor(command, code, stdout, stderr) { + super({ + message: 'command failed with error exit code', + info: { + command, + code + } + }) + this.stdout = stdout + this.stderr = stderr + this.code = code + } +} + +module.exports = { NotFoundError, FailedCommandError, ConversionsDisabledError } diff --git a/services/filestore/app/js/SafeExec.js b/services/filestore/app/js/SafeExec.js index dbc1576a88..5f079fa474 100644 --- a/services/filestore/app/js/SafeExec.js +++ b/services/filestore/app/js/SafeExec.js @@ -1,21 +1,8 @@ -/* eslint-disable - camelcase, - handle-callback-err, - no-return-assign, -*/ -// 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 - */ const _ = require('underscore') const logger = require('logger-sharelatex') -const child_process = require('child_process') +const childProcess = require('child_process') const Settings = require('settings-sharelatex') +const { ConversionsDisabledError, FailedCommandError } = require('./Errors') // execute a command in the same way as 'exec' but with a timeout that // kills all child processes @@ -23,36 +10,32 @@ const Settings = require('settings-sharelatex') // we spawn the command with 'detached:true' to make a new process // group, then we can kill everything in that process group. -module.exports = function(command, options, callback) { - if (callback == null) { - callback = function(err, stdout, stderr) {} - } +module.exports = safeExec +module.exports.promises = safeExecPromise + +// options are {timeout: number-of-milliseconds, killSignal: signal-name} +function safeExec(command, options, callback) { if (!Settings.enableConversions) { - const error = new Error('Image conversions are disabled') - return callback(error) + return callback( + new ConversionsDisabledError('image conversions are disabled') + ) } - // options are {timeout: number-of-milliseconds, killSignal: signal-name} - const [cmd, ...args] = Array.from(command) + const [cmd, ...args] = command - const child = child_process.spawn(cmd, args, { detached: true }) + const child = childProcess.spawn(cmd, args, { detached: true }) let stdout = '' let stderr = '' - const cleanup = _.once(function(err) { - if (killTimer != null) { - clearTimeout(killTimer) - } - return callback(err, stdout, stderr) - }) + let killTimer - if (options.timeout != null) { - var killTimer = setTimeout(function() { + if (options.timeout) { + killTimer = setTimeout(function() { try { // use negative process id to kill process group - return process.kill(-child.pid, options.killSignal || 'SIGTERM') + process.kill(-child.pid, options.killSignal || 'SIGTERM') } catch (error) { - return logger.log( + logger.log( { process: child.pid, kill_error: error }, 'error killing process' ) @@ -60,14 +43,41 @@ module.exports = function(command, options, callback) { }, options.timeout) } - child.on('close', function(code, signal) { - const err = code ? new Error(`exit status ${code}`) : signal - return cleanup(err) + const cleanup = _.once(function(err) { + if (killTimer) { + clearTimeout(killTimer) + } + callback(err, stdout, stderr) }) - child.on('error', err => cleanup(err)) + child.on('close', function(code, signal) { + if (code || signal) { + return cleanup( + new FailedCommandError(command, code || signal, stdout, stderr) + ) + } - child.stdout.on('data', chunk => (stdout += chunk)) + cleanup() + }) - return child.stderr.on('data', chunk => (stderr += chunk)) + child.on('error', err => { + cleanup(err) + }) + child.stdout.on('data', chunk => { + stdout += chunk + }) + child.stderr.on('data', chunk => { + stderr += chunk + }) +} + +function safeExecPromise(command, options) { + return new Promise((resolve, reject) => { + safeExec(command, options, (err, stdout, stderr) => { + if (err) { + reject(err) + } + resolve({ stdout, stderr }) + }) + }) } diff --git a/services/filestore/test/unit/js/SafeExecTests.js b/services/filestore/test/unit/js/SafeExecTests.js index 2b629947f5..077964ead7 100644 --- a/services/filestore/test/unit/js/SafeExecTests.js +++ b/services/filestore/test/unit/js/SafeExecTests.js @@ -1,95 +1,109 @@ -/* 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 { assert } = require('chai') -const sinon = require('sinon') const chai = require('chai') const should = chai.should() const { expect } = chai -const modulePath = '../../../app/js/SafeExec.js' +const modulePath = '../../../app/js/SafeExec' const SandboxedModule = require('sandboxed-module') describe('SafeExec', function() { + let settings, options, safeExec + beforeEach(function() { - this.settings = { enableConversions: true } - this.safe_exec = SandboxedModule.require(modulePath, { + settings = { enableConversions: true } + options = { timeout: 10 * 1000, killSignal: 'SIGTERM' } + + safeExec = SandboxedModule.require(modulePath, { requires: { 'logger-sharelatex': { log() {}, err() {} }, - 'settings-sharelatex': this.settings + 'settings-sharelatex': settings } }) - return (this.options = { timeout: 10 * 1000, killSignal: 'SIGTERM' }) }) - return describe('safe_exec', function() { + describe('safeExec', function() { it('should execute a valid command', function(done) { - return this.safe_exec( - ['/bin/echo', 'hello'], - this.options, - (err, stdout, stderr) => { - stdout.should.equal('hello\n') - should.not.exist(err) - return done() - } - ) + safeExec(['/bin/echo', 'hello'], options, (err, stdout, stderr) => { + stdout.should.equal('hello\n') + stderr.should.equal('') + should.not.exist(err) + done() + }) }) it('should error when conversions are disabled', function(done) { - this.settings.enableConversions = false - return this.safe_exec( - ['/bin/echo', 'hello'], - this.options, - (err, stdout, stderr) => { - expect(err).to.exist - return done() - } - ) + settings.enableConversions = false + safeExec(['/bin/echo', 'hello'], options, err => { + expect(err).to.exist + done() + }) }) it('should execute a command with non-zero exit status', function(done) { - return this.safe_exec( - ['/usr/bin/env', 'false'], - this.options, - (err, stdout, stderr) => { - stdout.should.equal('') - stderr.should.equal('') - err.message.should.equal('exit status 1') - return done() - } - ) + safeExec(['/usr/bin/env', 'false'], options, err => { + expect(err).to.exist + expect(err.name).to.equal('FailedCommandError') + expect(err.code).to.equal(1) + expect(err.stdout).to.equal('') + expect(err.stderr).to.equal('') + done() + }) }) it('should handle an invalid command', function(done) { - return this.safe_exec( - ['/bin/foobar'], - this.options, - (err, stdout, stderr) => { - err.code.should.equal('ENOENT') - return done() - } - ) + safeExec(['/bin/foobar'], options, err => { + err.code.should.equal('ENOENT') + done() + }) }) - return it('should handle a command that runs too long', function(done) { - return this.safe_exec( + it('should handle a command that runs too long', function(done) { + safeExec( ['/bin/sleep', '10'], { timeout: 500, killSignal: 'SIGTERM' }, - (err, stdout, stderr) => { - err.should.equal('SIGTERM') - return done() + err => { + expect(err).to.exist + expect(err.name).to.equal('FailedCommandError') + expect(err.code).to.equal('SIGTERM') + done() } ) }) }) + + describe('as a promise', function() { + beforeEach(function() { + safeExec = safeExec.promises + }) + + it('should execute a valid command', async function() { + const { stdout, stderr } = await safeExec(['/bin/echo', 'hello'], options) + + stdout.should.equal('hello\n') + stderr.should.equal('') + }) + + it('should throw a ConversionsDisabledError when appropriate', async function() { + settings.enableConversions = false + try { + await safeExec(['/bin/echo', 'hello'], options) + } catch (err) { + expect(err.name).to.equal('ConversionsDisabledError') + return + } + expect('method did not throw an error').not.to.exist + }) + + it('should throw a FailedCommandError when appropriate', async function() { + try { + await safeExec(['/usr/bin/env', 'false'], options) + } catch (err) { + expect(err.name).to.equal('FailedCommandError') + expect(err.code).to.equal(1) + return + } + expect('method did not throw an error').not.to.exist + }) + }) })