From 38f3511769c1f067f34f5d40cf0dd481049a8725 Mon Sep 17 00:00:00 2001 From: Carsten Klein Date: Wed, 12 Sep 2018 18:11:08 +0200 Subject: [PATCH] fix test for #121 --- .eslintrc.js | 3 +- lib/tmp.js | 107 +++++++++++++++++++----------- test/child-process.js | 31 +++++---- test/issue121-test.js | 49 +++++++++++--- test/issue129-sigint-test.js | 34 ++++++++++ test/outband/issue121.js | 23 +++---- test/outband/issue121.json | 1 + test/outband/issue129-sigint.js | 38 +++++++++++ test/outband/issue129-sigint.json | 3 + test/outband/issue129.js | 12 ++-- test/spawn-custom.js | 1 - test/spawn-generic.js | 2 +- test/spawn.js | 3 - 13 files changed, 221 insertions(+), 86 deletions(-) create mode 100644 test/issue129-sigint-test.js create mode 100644 test/outband/issue129-sigint.js create mode 100644 test/outband/issue129-sigint.json diff --git a/.eslintrc.js b/.eslintrc.js index 45d1d61..43ed1a1 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -23,6 +23,7 @@ module.exports = { "semi": [ "error", "always" - ] + ], + "no-extra-boolean-cast": 0 } }; diff --git a/lib/tmp.js b/lib/tmp.js index 8fe48a0..33d59bd 100644 --- a/lib/tmp.js +++ b/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,60 +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); } } - - // windows does not support signals - // it'd never had won if it wasn't a major PITA - // with node v8.x and win 10 this is no longer an issue - if (process.platform == 'win32') { - var rl = require('readline').createInterface({ - input: process.stdin, - output: process.stdout - }); - - rl.on('SIGINT', function () { - process.emit('SIGINT'); - }); - } - - process.on('SIGINT', function () { - process.exit(0); + 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); + } + } }); +} + +/** + * Safely install process exit listener. + * + * @private + */ +function _safely_install_exit_listener() { + const listeners = process.listeners(EXIT); - process.addListener(EVENT, function _tmp$safe_listener(data) { + // 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. diff --git a/test/child-process.js b/test/child-process.js index ce189e2..a519433 100644 --- a/test/child-process.js +++ b/test/child-process.js @@ -12,24 +12,25 @@ module.exports.genericChildProcess = _spawnProcess('spawn-generic.js'); module.exports.childProcess = _spawnProcess('spawn-custom.js'); function _spawnProcess(spawnFile) { - return function (testCase, configFile, cb) { - 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, @@ -48,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); } @@ -81,6 +79,12 @@ function _doSpawn(commandArgs, cb) { stderrDone = true; _close(); }); + + if (signal) { + setTimeout(function () { + child.kill(signal); + }, 2000); + } } function _bufferConcat(buffers) { @@ -89,10 +93,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; }, [])); } - diff --git a/test/issue121-test.js b/test/issue121-test.js index 69a24f3..16ff6ad 100644 --- a/test/issue121-test.js +++ b/test/issue121-test.js @@ -2,26 +2,57 @@ // vim: expandtab:ts=2:sw=2 const - assert = require('assert'), assertions = require('./assertions'), childProcess = require('./child-process').childProcess, - signals = ['SIGINT', 'SIGTERM']; + os = require('os'), + rimraf = require('rimraf'), + testCases = [ + { signal: 'SIGINT', expectExists: false }, + { signal: 'SIGTERM', expectExists: true } + ]; + +const isWindows = os.platform() === 'win32'; + +let tfunc = it; +if (isWindows) { + // skip tests on win32 + tfunc = xit; +} describe('tmp', function () { describe('issue121 - clean up on terminating signals', function () { - for (var i=0; i < signals.length; i++) { - issue121Tests(signals[i]); + 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) { +function issue121Tests(signal, expectExists) { return function (done) { - childProcess('issue121.json', function (err, stderr, stdout) { + childProcess(this, 'issue121.json', function (err, stderr, stdout) { if (err) return done(err); else if (stderr) return done(new Error(stderr)); - else assertions.assertDoesNotExist(stdout); - done(); - }, true); + + try { + if (expectExists) { + assertions.assertExists(stdout); + } + else { + assertions.assertDoesNotExist(stdout); + } + done(); + } catch (err) { + done(err); + } finally { + // cleanup + if (expectExists) { + rimraf.sync(stdout); + } + } + }, signal); }; } diff --git a/test/issue129-sigint-test.js b/test/issue129-sigint-test.js new file mode 100644 index 0000000..0ba9b82 --- /dev/null +++ b/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(); + }); + }); + }); +}); diff --git a/test/outband/issue121.js b/test/outband/issue121.js index 0dce377..deba395 100644 --- a/test/outband/issue121.js +++ b/test/outband/issue121.js @@ -1,20 +1,19 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var - fs = require('fs'), - tmp = require('../../lib/tmp'), - // we reuse the fixtures from issue62 here - fixture = require('./issue62'); - -tmp.setGracefulCleanup(); +const + tmp = require('../../lib/tmp'); // https://github.com/raszi/node-tmp/issues/121 -module.exports = function (signal) { - fixture.apply(this, [tmp.dirSync({ unsafeCleanup: true }), tmp]); +module.exports = function () { + + tmp.setGracefulCleanup(); + + const result = tmp.dirSync({ unsafeCleanup: true }); - // make sure that the process keeps running - setTimeout(function () {}, 1000000); + this.out(result.name, function () { }); - this.kill(signal); + setTimeout(function () { + throw new Error('ran into timeout'); + }, 10000); }; diff --git a/test/outband/issue121.json b/test/outband/issue121.json index 86429e8..2210135 100644 --- a/test/outband/issue121.json +++ b/test/outband/issue121.json @@ -1,3 +1,4 @@ { + "graceful": true, "tc": "issue121" } diff --git a/test/outband/issue129-sigint.js b/test/outband/issue129-sigint.js new file mode 100644 index 0000000..0710536 --- /dev/null +++ b/test/outband/issue129-sigint.js @@ -0,0 +1,38 @@ +/* eslint-disable no-octal */ +// vim: expandtab:ts=2:sw=2 + +// addendum to https://github.com/raszi/node-tmp/issues/129 so that with jest sandboxing we do not install our sigint +// listener multiple times +module.exports = function () { + var callState = { + existingListener : false, + }; + + // simulate an existing SIGINT listener + var listener1 = (function (callState) { + return function _tmp$sigint_listener(doExit) { + callState.existingListener = !doExit; + }; + })(callState); + + process.addListener('SIGINT', listener1); + + // now let tmp install its listener safely + require('../../lib/tmp'); + + var sigintListeners = []; + + var listeners = process.listeners('SIGINT'); + for (var i = 0; i < listeners.length; i++) { + var listener = listeners[i]; + if (listener.name === '_tmp$sigint_listener') { + sigintListeners.push(listener); + } + } + + if (listeners.length > 1) this.fail('EEXISTS:MULTIPLE: existing SIGINT listener was not removed', this.exit); + listeners[0](false); // prevent listener from exiting the process + if (!callState.existingListener) this.fail('ENOAVAIL:EXISTING: existing listener was not called', this.exit); + this.out('EOK', this.exit); + process.exit(0); +}; diff --git a/test/outband/issue129-sigint.json b/test/outband/issue129-sigint.json new file mode 100644 index 0000000..967ff91 --- /dev/null +++ b/test/outband/issue129-sigint.json @@ -0,0 +1,3 @@ +{ + "tc": "issue129-sigint" +} diff --git a/test/outband/issue129.js b/test/outband/issue129.js index 0d65547..a36c56b 100644 --- a/test/outband/issue129.js +++ b/test/outband/issue129.js @@ -5,10 +5,10 @@ module.exports = function () { // dup from lib/tmp.js function _is_legacy_listener(listener) { - return (listener.name == '_exit' || listener.name == '_uncaughtExceptionThrown') - && listener.toString().indexOf('_garbageCollector();') != -1; + return (listener.name === '_exit' || listener.name === '_uncaughtExceptionThrown') + && listener.toString().indexOf('_garbageCollector();') !== -1; } - +å function _garbageCollector() {} var callState = { @@ -56,14 +56,14 @@ module.exports = function () { for (var i = 0; i < listeners.length; i++) { var listener = listeners[i]; // the order here is important - if (listener.name == '_tmp$safe_listener') { + if (listener.name === '_tmp$safe_listener') { newStyleListeners.push(listener); } else if (_is_legacy_listener(listener)) { - if (listener.name == '_uncaughtExceptionThrown') legacyUncaughtListener = listener; + if (listener.name === '_uncaughtExceptionThrown') legacyUncaughtListener = listener; else legacyExitListener = listener; } - } + }å if (legacyExitListener) this.fail('EEXISTS:LEGACY:EXIT existing legacy exit listener was not removed', this.exit); if (legacyUncaughtListener) this.fail('EEXISTS:LEGACY:UNCAUGHT existing legacy uncaught exception thrown listener was not removed', this.exit); diff --git a/test/spawn-custom.js b/test/spawn-custom.js index bb1cc27..f833f07 100644 --- a/test/spawn-custom.js +++ b/test/spawn-custom.js @@ -11,4 +11,3 @@ spawn.graceful = !!config.graceful; // import the test case function and execute it var fn = require(path.join(__dirname, 'outband', config.tc)); fn.apply(spawn); - diff --git a/test/spawn-generic.js b/test/spawn-generic.js index 6b4cb5b..d557db0 100644 --- a/test/spawn-generic.js +++ b/test/spawn-generic.js @@ -23,7 +23,7 @@ if (config.async) fnUnderTest(config.options, function (err, name, fdOrCallback, cb) { if (err) spawn.err(err); else { - var result = null; + var result = null; if (config.file) result = { name: name, fd: fdOrCallback, removeCallback: cb }; else result = { name: name, removeCallback: fdOrCallback }; fn.apply(spawn, [result, tmp]); diff --git a/test/spawn.js b/test/spawn.js index 5dfd87e..3f89510 100644 --- a/test/spawn.js +++ b/test/spawn.js @@ -28,9 +28,6 @@ module.exports = { }, exit: function (code) { process.exit(code || 0); - }, - kill: function (signal) { - process.kill(signal || 'SIGINT'); } };