Merge pull request #159 from raszi/gh-121

fix #121

Author: silkentrance

.eslintrc.js

@@ -23,6 +23,7 @@ module.exports = {
     "semi": [
       "error",
       "always"
-    ]
+    ],
+    "no-extra-boolean-cast": 0
   }
 };

.travis.yml

@@ -4,7 +4,6 @@ node_js:
   - "10"
   - "9"
   - "8"
-  - "6"
 sudo: false
 cache:
   directories:

appveyor.yml

@@ -2,11 +2,11 @@
 
 environment:
   matrix:
-    - nodejs_version: "6"
     - nodejs_version: "7"
     - nodejs_version: "8"
     - nodejs_version: "9"
     - nodejs_version: "10"
+    - nodejs_version: "11"
 
 install:
   - ps: Install-Product node $env:nodejs_version

lib/tmp.js

@@ -43,7 +43,9 @@ const
   DIR_MODE = 448 /* 0o700 */,
   FILE_MODE = 384 /* 0o600 */,
 
-  EVENT = 'exit',
+  EXIT = 'exit',
+
+  SIGINT = 'SIGINT',
 
   // this will hold the objects need to be removed on exit
   _removeObjects = [];
@@ -101,7 +103,7 @@ function _isUndefined(obj) {
  */
 function _parseArguments(options, callback) {
   /* istanbul ignore else */
-  if (typeof options == 'function') {
+  if (typeof options === 'function') {
     return [{}, options];
   }
 
@@ -132,7 +134,7 @@ function _generateTmpName(opts) {
     var template = opts.template;
     // make sure that we prepend the tmp path if none was given
     /* istanbul ignore else */
-    if (path.basename(template) == template)
+    if (path.basename(template) === template)
       template = path.join(opts.dir || tmpDir, template);
     return template.replace(TEMPLATE_PATTERN, _randomChars(6));
   }
@@ -479,7 +481,7 @@ function _prepareRemoveCallback(removeFunction, arg, cleanupCallbackSync) {
 
       called = true;
       // sync?
-      if (removeFunction.length == 1) {
+      if (removeFunction.length === 1) {
         try {
           removeFunction(arg);
           return next(null);
@@ -550,7 +552,7 @@ function isENOENT(error) {
  *   error.errno n/a
  */
 function isExpectedError(error, code, errno) {
-  return error.code == code || error.code == errno;
+  return error.code === code || error.code === errno;
 }
 
 /**
@@ -569,43 +571,87 @@ function setGracefulCleanup() {
  * @returns {Boolean} true whether listener is a legacy listener
  */
 function _is_legacy_listener(listener) {
-  return (listener.name == '_exit' || listener.name == '_uncaughtExceptionThrown')
+  return (listener.name === '_exit' || listener.name === '_uncaughtExceptionThrown')
     && listener.toString().indexOf('_garbageCollector();') > -1;
 }
 
 /**
- * Safely install process exit listeners.
- * 
+ * Safely install SIGINT listener.
+ *
+ * NOTE: this will only work on OSX and Linux.
+ *
  * @private
  */
-function _safely_install_listener() {
-  var listeners = process.listeners(EVENT);
+function _safely_install_sigint_listener() {
 
-  // collect any existing listeners
-  var existingListeners = [];
-  for (var i = 0, length = listeners.length; i < length; i++) {
-    var lstnr = listeners[i];
+  const listeners = process.listeners(SIGINT);
+  const existingListeners = [];
+  for (let i = 0, length = listeners.length; i < length; i++) {
+    const lstnr = listeners[i];
     /* istanbul ignore else */
-    if (lstnr.name == '_tmp$safe_listener' || _is_legacy_listener(lstnr)) {
-      // we must forget about the uncaughtException listener
-      if (lstnr.name != '_uncaughtExceptionThrown') existingListeners.push(lstnr);
-      process.removeListener(EVENT, lstnr);
+    if (lstnr.name === '_tmp$sigint_listener') {
+      existingListeners.push(lstnr);
+      process.removeListener(SIGINT, lstnr);
     }
   }
+  process.on(SIGINT, function _tmp$sigint_listener(doExit) {
+    for (let i = 0, length = existingListeners.length; i < length; i++) {
+      // let the existing listener do the garbage collection (e.g. jest sandbox)
+      try {
+        existingListeners[i](false);
+      } catch (err) {
+        // ignore
+      }
+    }
+    try {
+      // force the garbage collector even it is called again in the exit listener
+      _garbageCollector();
+    } finally {
+      if (!!doExit) {
+        process.exit(0);
+      }
+    }
+  });
+}
 
-  process.addListener(EVENT, function _tmp$safe_listener(data) {
+/**
+ * Safely install process exit listener.
+ *
+ * @private
+ */
+function _safely_install_exit_listener() {
+  const listeners = process.listeners(EXIT);
+
+  // collect any existing listeners
+  const existingListeners = [];
+  for (let i = 0, length = listeners.length; i < length; i++) {
+    const lstnr = listeners[i];
     /* istanbul ignore else */
-    if (existingListeners.length) {
-      for (var i = 0, length = existingListeners.length; i < length; i++) {
+    // TODO: remove support for legacy listeners once release 1.0.0 is out
+    if (lstnr.name === '_tmp$safe_listener' || _is_legacy_listener(lstnr)) {
+      // we must forget about the uncaughtException listener, hopefully it is ours
+      if (lstnr.name !== '_uncaughtExceptionThrown') {
+        existingListeners.push(lstnr);
+      }
+      process.removeListener(EXIT, lstnr);
+    }
+  }
+  // TODO: what was the data parameter good for?
+  process.addListener(EXIT, function _tmp$safe_listener(data) {
+    for (let i = 0, length = existingListeners.length; i < length; i++) {
+      // let the existing listener do the garbage collection (e.g. jest sandbox)
+      try {
         existingListeners[i](data);
+      } catch (err) {
+        // ignore
       }
     }
     _garbageCollector();
   });
 }
 
-_safely_install_listener();
-
+_safely_install_exit_listener();
+_safely_install_sigint_listener();
 
 /**
  * Configuration options.

package.json

@@ -38,7 +38,7 @@
   "scripts": {
     "lint": "eslint lib --env mocha test",
     "clean": "rm -Rf ./coverage",
-    "test": "istanbul cover ./node_modules/mocha/bin/_mocha --report none --print none --dir ./coverage/json -- -u exports test/*-test.js && istanbul report --root ./coverage/json html && istanbul report text-summary",
+    "test": "npm run clean && istanbul cover ./node_modules/mocha/bin/_mocha --report none --print none --dir ./coverage/json -u exports -R test/*-test.js && istanbul report --root ./coverage/json html && istanbul report text-summary",
     "doc": "jsdoc -c .jsdoc.json"
   }
 }

test/child-process.js

@@ -12,26 +12,25 @@ module.exports.genericChildProcess = _spawnProcess('spawn-generic.js');
 module.exports.childProcess = _spawnProcess('spawn-custom.js');
 
 function _spawnProcess(spawnFile) {
-  return function (testCase, configFile, cb) {
-    testCase.timeout(5000);
-
-    var
+  return function (testCase, configFile, cb, signal) {
+    const
       configFilePath = path.join(__dirname, 'outband', configFile),
       commandArgs = [path.join(__dirname, spawnFile), configFilePath];
 
     exists(configFilePath, function (configExists) {
-      if (configExists) return _doSpawn(commandArgs, cb);
+      if (configExists) return _doSpawn(commandArgs, cb, signal);
 
       cb(new Error('ENOENT: configFile ' + configFilePath + ' does not exist'));
     });
   };
 }
 
-function _doSpawn(commandArgs, cb) {
-  var
+function _doSpawn(commandArgs, cb, signal) {
+  const
     node_path = process.argv[0],
     stdoutBufs = [],
-    stderrBufs = [],
+    stderrBufs = [];
+  let
     child,
     done = false,
     stderrDone = false,
@@ -50,14 +49,11 @@ function _doSpawn(commandArgs, cb) {
   child = spawn(node_path, commandArgs);
   child.stdin.end();
 
-  // TODO we no longer support node 0.6
-  // Cannot use 'close' event because not on node-0.6.
   function _close() {
-    var
-      stderr = _bufferConcat(stderrBufs).toString(),
-      stdout = _bufferConcat(stdoutBufs).toString();
-
     if (stderrDone && stdoutDone && !done) {
+      const
+        stderr = _bufferConcat(stderrBufs).toString(),
+        stdout = _bufferConcat(stdoutBufs).toString();
       done = true;
       cb(null, stderr, stdout);
     }
@@ -83,6 +79,13 @@ function _doSpawn(commandArgs, cb) {
     stderrDone = true;
     _close();
   });
+
+  if (signal) {
+    setTimeout(function () {
+      // does not work on node 6
+      child.kill(signal);
+    }, 2000);
+  }
 }
 
 function _bufferConcat(buffers) {
@@ -91,10 +94,9 @@ function _bufferConcat(buffers) {
   }
 
   return new Buffer(buffers.reduce(function (acc, buf) {
-    for (var i = 0; i < buf.length; i++) {
+    for (let i = 0; i < buf.length; i++) {
       acc.push(buf[i]);
     }
     return acc;
   }, []));
 }
-

test/dir-sync-test.js

@@ -125,10 +125,11 @@ describe('tmp', function () {
           try {
             assertions.assertExists(stdout);
             assertions.assertExists(path.join(stdout, 'should-be-removed.file'), true);
-            if (process.platform == 'win32')
+            if (process.platform == 'win32') {
               assertions.assertExists(path.join(stdout, 'symlinkme-target'), true);
-            else
+            } else {
               assertions.assertExists(path.join(stdout, 'symlinkme-target'));
+            }
           } catch (err) {
             rimraf.sync(stdout);
             return done(err);

test/issue121-test.js

@@ -0,0 +1,54 @@
+/* eslint-disable no-octal */
+// vim: expandtab:ts=2:sw=2
+
+const
+  assertions = require('./assertions'),
+  childProcess = require('./child-process').childProcess,
+  os = require('os'),
+  rimraf = require('rimraf'),
+  testCases = [
+    { signal: 'SIGINT', expectExists: false },
+    { signal: 'SIGTERM', expectExists: true }
+  ];
+
+// skip tests on win32
+const isWindows = os.platform() === 'win32';
+const tfunc = isWindows ? xit : it;
+
+describe('tmp', function () {
+  describe('issue121 - clean up on terminating signals', function () {
+    for (let tc of testCases) {
+      tfunc('for signal ' + tc.signal, function (done) {
+        // increase timeout so that the child process may fail
+        this.timeout(20000);
+        issue121Tests(tc.signal, tc.expectExists)(done);
+      });
+    }
+  });
+});
+
+function issue121Tests(signal, expectExists) {
+  return function (done) {
+    childProcess(this, 'issue121.json', function (err, stderr, stdout) {
+      if (err) return done(err);
+      else if (stderr) return done(new Error(stderr));
+
+      try {
+        if (expectExists) {
+          assertions.assertExists(stdout);
+        }
+        else {
+          assertions.assertDoesNotExist(stdout);
+        }
+        done();
+      } catch (err) {
+        done(err);
+      } finally {
+        // cleanup
+        if (expectExists) {
+          rimraf.sync(stdout);
+        }
+      }
+    }, signal);
+  };
+}

test/issue129-sigint-test.js

@@ -0,0 +1,34 @@
+/* eslint-disable no-octal */
+// vim: expandtab:ts=2:sw=2
+
+var
+  assert = require('assert'),
+  assertions = require('./assertions'),
+  childProcess = require('./child-process').childProcess;
+
+describe('tmp', function () {
+  describe('issue129: safely install sigint listeners', function () {
+    it('when simulating sandboxed behavior', function (done) {
+      childProcess(this, 'issue129-sigint.json', function (err, stderr, stdout) {
+        if (err) return done(err);
+        if (!stdout && !stderr) return done(new Error('stderr or stdout expected'));
+        if (stderr) {
+          try {
+            assertions.assertDoesNotStartWith(stderr, 'EEXISTS:MULTIPLE');
+            assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:EXISTING');
+          } catch (err) {
+            return done(err);
+          }
+        }
+        if (stdout) {
+          try {
+            assert.equal(stdout, 'EOK', 'existing listeners should have been removed and called');
+          } catch (err) {
+            return done(err);
+          }
+        }
+        done();
+      });
+    });
+  });
+});

test/outband/issue121.js

@@ -0,0 +1,19 @@
+/* eslint-disable no-octal */
+// vim: expandtab:ts=2:sw=2
+
+const
+  tmp = require('../../lib/tmp');
+
+// https://github.com/raszi/node-tmp/issues/121
+module.exports = function () {
+
+  tmp.setGracefulCleanup();
+
+  const result = tmp.dirSync({ unsafeCleanup: true });
+
+  this.out(result.name, function () { });
+
+  setTimeout(function () {
+    throw new Error('ran into timeout');
+  }, 10000);
+};

test/outband/issue121.json

@@ -0,0 +1,4 @@
+{
+  "graceful": true,
+  "tc": "issue121"
+}