Remove separate "internal error" from exec (#802)

* Remove separate "internal error" from exec

* Fix unknown command error regex

* Add message about command not found regex

* Silence errors while reading files in exec

The stdout and stderr files may never be opened or written to in certain
circumstances. In particular, if the timeout is short enough, the child
node process does not have enough time to start, and the child script
does not execute, so the files are not written to. So, catch errors form
trying to read the files, and ignore them.

* Do not silence errors due to short timeouts

* Simplify test regex for missing command

* Default error code to 1 if not set

src/exec.js

@@ -6,6 +6,7 @@ var fs = require('fs');
 var child = require('child_process');
 
 var DEFAULT_MAXBUFFER_SIZE = 20 * 1024 * 1024;
+var DEFAULT_ERROR_CODE = 1;
 
 common.register('exec', _exec, {
   unix: false,
@@ -72,13 +73,15 @@ function execSync(cmd, opts, pipe) {
     child.execFileSync(common.config.execPath, execArgs, opts);
   } catch (e) {
     // Commands with non-zero exit code raise an exception.
-    code = e.status;
+    code = e.status || DEFAULT_ERROR_CODE;
   }
 
   // fs.readFileSync uses buffer encoding by default, so call
-  // it without the encoding option if the encoding is 'buffer'
-  var stdout;
-  var stderr;
+  // it without the encoding option if the encoding is 'buffer'.
+  // Also, if the exec timeout is too short for node to start up,
+  // the files will not be created, so these calls will throw.
+  var stdout = '';
+  var stderr = '';
   if (opts.encoding === 'buffer') {
     stdout = fs.readFileSync(stdoutFile);
     stderr = fs.readFileSync(stderrFile);
@@ -93,7 +96,7 @@ function execSync(cmd, opts, pipe) {
   try { common.unlinkSync(stdoutFile); } catch (e) {}
 
   if (code !== 0) {
-    common.error('', code, { continue: true });
+    common.error(stderr, code, { continue: true });
   }
   var obj = common.ShellString(stdout, stderr, code);
   return obj;
@@ -196,14 +199,10 @@ function _exec(command, options, callback) {
     async: false,
   }, options);
 
-  try {
-    if (options.async) {
-      return execAsync(command, options, pipe, callback);
-    } else {
-      return execSync(command, options, pipe);
-    }
-  } catch (e) {
-    common.error('internal error');
+  if (options.async) {
+    return execAsync(command, options, pipe, callback);
+  } else {
+    return execSync(command, options, pipe);
   }
 }
 module.exports = _exec;

test/exec.js

@@ -35,7 +35,7 @@ test('config.fatal and unknown command', t => {
   shell.config.fatal = true;
   t.throws(() => {
     shell.exec('asdfasdf'); // could not find command
-  }, /exec: internal error/);
+  }, /asdfasdf/); // name of command should be in error message
   shell.config.fatal = oldFatal;
 });
 
@@ -127,7 +127,7 @@ test('set timeout option', t => {
   const result = shell.exec(`${JSON.stringify(shell.config.execPath)} test/resources/exec/slow.js 100`); // default timeout is ok
   t.falsy(shell.error());
   t.is(result.code, 0);
-  shell.exec(`${JSON.stringify(shell.config.execPath)} test/resources/exec/slow.js 100`, { timeout: 10 }); // times out
+  shell.exec(`${JSON.stringify(shell.config.execPath)} test/resources/exec/slow.js 2000`, { timeout: 1000 }); // times out
   t.truthy(shell.error());
 });