refactor(exec): move child process to source file (#786)

This PR refactors `shell.exec()` by putting its child process in a separate code
file. This also slightly cleans up dead code.

There's more potential to clean this up (e.g. exit status), but this is a good
enough start.

Issue #782

src/exec-child.js

@@ -0,0 +1,46 @@
+if (require.main !== module) {
+  throw new Error('This file should not be required');
+}
+
+var childProcess = require('child_process');
+var fs = require('fs');
+
+// Note: this will break if `paramFilePath` contains special characters ( '\n',
+// '\t', etc.). Such characters are possible if $TMP gets modified. We already
+// rely on tempdir() to work for other things, so this is an acceptable risk.
+var paramFilePath = process.argv[2];
+
+var serializedParams = fs.readFileSync(paramFilePath, 'utf8');
+var params = JSON.parse(serializedParams);
+
+var cmd = params.command;
+var execOptions = params.execOptions;
+var pipe = params.pipe;
+var stdoutFile = params.stdoutFile;
+var stderrFile = params.stderrFile;
+var codeFile = params.codeFile;
+
+var c = childProcess.exec(cmd, execOptions, function (err) {
+  if (!err) {
+    fs.writeFileSync(codeFile, '0');
+  } else if (err.code === undefined) {
+    fs.writeFileSync(codeFile, '1');
+  } else {
+    fs.writeFileSync(codeFile, err.code.toString());
+  }
+});
+
+var stdoutStream = fs.createWriteStream(stdoutFile);
+var stderrStream = fs.createWriteStream(stderrFile);
+
+c.stdout.pipe(stdoutStream, { end: false });
+c.stderr.pipe(stderrStream, { end: false });
+c.stdout.pipe(process.stdout);
+c.stderr.pipe(process.stderr);
+
+if (pipe) {
+  c.stdin.end(pipe);
+}
+
+c.stdout.on('end', stdoutStream.end);
+c.stderr.on('end', stderrStream.end);

src/exec.js

@@ -24,10 +24,10 @@ function execSync(cmd, opts, pipe) {
   }
 
   var tempDir = _tempDir();
-  var stdoutFile = path.resolve(tempDir + '/' + common.randomFileName());
-  var stderrFile = path.resolve(tempDir + '/' + common.randomFileName());
   var codeFile = path.resolve(tempDir + '/' + common.randomFileName());
-  var scriptFile = path.resolve(tempDir + '/' + common.randomFileName());
+  var paramsFile = path.resolve(tempDir + '/' + common.randomFileName());
+  var stderrFile = path.resolve(tempDir + '/' + common.randomFileName());
+  var stdoutFile = path.resolve(tempDir + '/' + common.randomFileName());
 
   opts = common.extend({
     silent: common.config.silent,
@@ -37,47 +37,29 @@ function execSync(cmd, opts, pipe) {
     encoding: 'utf8',
   }, opts);
 
-  if (fs.existsSync(scriptFile)) common.unlinkSync(scriptFile);
-  if (fs.existsSync(stdoutFile)) common.unlinkSync(stdoutFile);
-  if (fs.existsSync(stderrFile)) common.unlinkSync(stderrFile);
   if (fs.existsSync(codeFile)) common.unlinkSync(codeFile);
-
-  var execCommand = JSON.stringify(common.config.execPath) + ' ' + JSON.stringify(scriptFile);
-  var script;
+  if (fs.existsSync(paramsFile)) common.unlinkSync(paramsFile);
+  if (fs.existsSync(stderrFile)) common.unlinkSync(stderrFile);
+  if (fs.existsSync(stdoutFile)) common.unlinkSync(stdoutFile);
 
   opts.cwd = path.resolve(opts.cwd);
-  var optString = JSON.stringify(opts);
-
-  script = [
-    "var child = require('child_process')",
-    "  , fs = require('fs');",
-    'var childProcess = child.exec(' + JSON.stringify(cmd) + ', ' + optString + ', function(err) {',
-    '  var fname = ' + JSON.stringify(codeFile) + ';',
-    '  if (!err) {',
-    '    fs.writeFileSync(fname, "0");',
-    '  } else if (err.code === undefined) {',
-    '    fs.writeFileSync(fname, "1");',
-    '  } else {',
-    '    fs.writeFileSync(fname, err.code.toString());',
-    '  }',
-    '});',
-    'var stdoutStream = fs.createWriteStream(' + JSON.stringify(stdoutFile) + ');',
-    'var stderrStream = fs.createWriteStream(' + JSON.stringify(stderrFile) + ');',
-    'childProcess.stdout.pipe(stdoutStream, {end: false});',
-    'childProcess.stderr.pipe(stderrStream, {end: false});',
-    'childProcess.stdout.pipe(process.stdout);',
-    'childProcess.stderr.pipe(process.stderr);',
-  ].join('\n') +
-    (pipe ? '\nchildProcess.stdin.end(' + JSON.stringify(pipe) + ');\n' : '\n') +
-    [
-      'var stdoutEnded = false, stderrEnded = false;',
-      'function tryClosingStdout(){ if(stdoutEnded){ stdoutStream.end(); } }',
-      'function tryClosingStderr(){ if(stderrEnded){ stderrStream.end(); } }',
-      "childProcess.stdout.on('end', function(){ stdoutEnded = true; tryClosingStdout(); });",
-      "childProcess.stderr.on('end', function(){ stderrEnded = true; tryClosingStderr(); });",
-    ].join('\n');
-
-  fs.writeFileSync(scriptFile, script);
+
+  var paramsToSerialize = {
+    command: cmd,
+    execOptions: opts,
+    pipe: pipe,
+    stdoutFile: stdoutFile,
+    stderrFile: stderrFile,
+    codeFile: codeFile,
+  };
+
+  fs.writeFileSync(paramsFile, JSON.stringify(paramsToSerialize), 'utf8');
+
+  var execCommand = [
+    JSON.stringify(common.config.execPath),
+    JSON.stringify(path.join(__dirname, 'exec-child.js')),
+    JSON.stringify(paramsFile),
+  ].join(' ');
 
   /* istanbul ignore else */
   if (opts.silent) {
@@ -91,10 +73,10 @@ function execSync(cmd, opts, pipe) {
     child.execSync(execCommand, opts);
   } catch (e) {
     // Clean up immediately if we have an exception
-    try { common.unlinkSync(scriptFile); } catch (e2) {}
-    try { common.unlinkSync(stdoutFile); } catch (e2) {}
-    try { common.unlinkSync(stderrFile); } catch (e2) {}
     try { common.unlinkSync(codeFile); } catch (e2) {}
+    try { common.unlinkSync(paramsFile); } catch (e2) {}
+    try { common.unlinkSync(stderrFile); } catch (e2) {}
+    try { common.unlinkSync(stdoutFile); } catch (e2) {}
     throw e;
   }
 
@@ -118,10 +100,10 @@ function execSync(cmd, opts, pipe) {
   }
 
   // No biggie if we can't erase the files now -- they're in a temp dir anyway
-  try { common.unlinkSync(scriptFile); } catch (e) {}
-  try { common.unlinkSync(stdoutFile); } catch (e) {}
-  try { common.unlinkSync(stderrFile); } catch (e) {}
   try { common.unlinkSync(codeFile); } catch (e) {}
+  try { common.unlinkSync(paramsFile); } catch (e) {}
+  try { common.unlinkSync(stderrFile); } catch (e) {}
+  try { common.unlinkSync(stdoutFile); } catch (e) {}
 
   if (code !== 0) {
     common.error('', code, { continue: true });

test/exec.js

@@ -48,6 +48,12 @@ test('exec exits gracefully if we cannot find the execPath', t => {
   );
 });
 
+test('cannot require exec-child.js', t => {
+  t.throws(() => {
+    require('../src/exec-child.js');
+  }, /This file should not be required/);
+});
+
 //
 // Valids
 //