Cleanup SafeExec

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 }

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

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