From 9275e3e9847df4950c549f5b12d4b54083ad7b96 Mon Sep 17 00:00:00 2001 From: Carsten Klein Date: Fri, 26 Apr 2019 15:19:41 +0200 Subject: [PATCH] fix #192: tmp must not exit the process on its own refactor rename data parameter on exit listener to code --- lib/tmp.js | 45 ++++++++++++++++++++------------- test/issue121-test.js | 30 ++++++---------------- test/issue129-sigint-test.js | 19 +++++--------- test/issue129-test.js | 22 +++++----------- test/outband/issue121.js | 4 +++ test/outband/issue129-sigint.js | 24 +++++++----------- test/outband/issue129.js | 4 +-- 7 files changed, 64 insertions(+), 84 deletions(-) diff --git a/lib/tmp.js b/lib/tmp.js index 2bc50ab..1af6031 100644 --- a/lib/tmp.js +++ b/lib/tmp.js @@ -41,6 +41,8 @@ const SIGINT = 'SIGINT', + SIGINT_LISTENER_NAME = '_tmp$sigint_listener' + // this will hold the objects need to be removed on exit _removeObjects = []; @@ -608,33 +610,43 @@ function _is_legacy_listener(listener) { */ function _safely_install_sigint_listener() { - const listeners = process.listeners(SIGINT); + let 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$sigint_listener') { + if (lstnr.name === SIGINT_LISTENER_NAME) { existingListeners.push(lstnr); process.removeListener(SIGINT, lstnr); } } - process.on(SIGINT, function _tmp$sigint_listener(doExit) { + process.on(SIGINT, function _tmp$sigint_listener() { 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); + // let the existing listener do the garbage collection (e.g. jest sandbox) + // pass in false in case that we are dealing with an old version of this + 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); - } - } + // check if there are any user installed sigint listeners + // if there are none, exit the process + let doExit = true; + listeners = process.listeners(SIGINT); + for (let i = 0, length = listeners.length; i < length; i++) { + const lstnr = listeners[i]; + if (lstnr.name !== SIGINT_LISTENER_NAME) { + doExit = false; + break; + } + } + // force the garbage collector even it is called again in the exit listener + _garbageCollector(); + // exit the process if no user listeners have been installed + if (doExit) { + process.exit(); + } }); } @@ -660,12 +672,11 @@ function _safely_install_exit_listener() { process.removeListener(EXIT, lstnr); } } - // TODO: what was the data parameter good for? - process.addListener(EXIT, function _tmp$safe_listener(data) { + process.addListener(EXIT, function _tmp$safe_listener(code) { 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); + // let the existing listener do the garbage collection (e.g. jest sandbox) + existingListeners[i](code); } catch (err) { // ignore } diff --git a/test/issue121-test.js b/test/issue121-test.js index 4eaddcb..5d0e3bb 100644 --- a/test/issue121-test.js +++ b/test/issue121-test.js @@ -7,8 +7,8 @@ const os = require('os'), rimraf = require('rimraf'), testCases = [ - { signal: 'SIGINT', expectExists: false }, - { signal: 'SIGTERM', expectExists: true } + 'SIGINT', + 'SIGTERM' ]; // skip tests on win32 @@ -18,10 +18,10 @@ 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); + tfunc('for signal ' + tc, function (done) { + // increase timeout so that the child process may terminate in time + this.timeout(5000); + issue121Tests(tc)(done); }); } }); @@ -33,22 +33,8 @@ function issue121Tests(signal, expectExists) { 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); - } - } + assertions.assertDoesNotExist(stdout); + done(); }, signal); }; } diff --git a/test/issue129-sigint-test.js b/test/issue129-sigint-test.js index 0ba9b82..30880d9 100644 --- a/test/issue129-sigint-test.js +++ b/test/issue129-sigint-test.js @@ -11,23 +11,16 @@ describe('tmp', 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); - } + assertions.assertDoesNotStartWith(stderr, 'EEXISTS:MULTIPLE'); + assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:'); + return done(); } if (stdout) { - try { - assert.equal(stdout, 'EOK', 'existing listeners should have been removed and called'); - } catch (err) { - return done(err); - } + assert.equal(stdout, 'EOK'); + return done(); } - done(); + done(new Error('existing listener has not been called')); }); }); }); diff --git a/test/issue129-test.js b/test/issue129-test.js index c157cb3..250483a 100644 --- a/test/issue129-test.js +++ b/test/issue129-test.js @@ -13,23 +13,15 @@ describe('tmp', function () { if (err) return done(err); if (!stdout && !stderr) return done(new Error('stderr or stdout expected')); if (stderr) { - try { - assertions.assertDoesNotStartWith(stderr, 'EEXISTS:LEGACY:EXIT'); - assertions.assertDoesNotStartWith(stderr, 'EEXISTS:LEGACY:UNCAUGHT'); - assertions.assertDoesNotStartWith(stderr, 'EEXISTS:NEWSTYLE'); - assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:LEGACY:EXIT'); - assertions.assertDoesNotStartWith(stderr, 'EAVAIL:LEGACY:UNCAUGHT'); - assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:NEWSTYLE'); - } catch (err) { - return done(err); - } + assertions.assertDoesNotStartWith(stderr, 'EEXISTS:LEGACY:EXIT'); + assertions.assertDoesNotStartWith(stderr, 'EEXISTS:LEGACY:UNCAUGHT'); + assertions.assertDoesNotStartWith(stderr, 'EEXISTS:NEWSTYLE'); + assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:LEGACY:EXIT'); + assertions.assertDoesNotStartWith(stderr, 'EAVAIL:LEGACY:UNCAUGHT'); + assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:NEWSTYLE'); } if (stdout) { - try { - assert.equal(stdout, 'EOK', 'existing listeners should have been removed and called'); - } catch (err) { - return done(err); - } + assert.equal(stdout, 'EOK', 'existing listeners should have been removed and called'); } done(); }); diff --git a/test/outband/issue121.js b/test/outband/issue121.js index deba395..d941d78 100644 --- a/test/outband/issue121.js +++ b/test/outband/issue121.js @@ -4,6 +4,10 @@ const tmp = require('../../lib/tmp'); +process.on('SIGTERM', function () { + process.exit(0); +}); + // https://github.com/raszi/node-tmp/issues/121 module.exports = function () { diff --git a/test/outband/issue129-sigint.js b/test/outband/issue129-sigint.js index 0710536..f3769fb 100644 --- a/test/outband/issue129-sigint.js +++ b/test/outband/issue129-sigint.js @@ -4,18 +4,13 @@ // 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); + var self = this; - process.addListener('SIGINT', listener1); + // simulate an existing SIGINT listener + process.addListener('SIGINT', function _tmp$sigint_listener() { + self.out('EOK'); + }); // now let tmp install its listener safely require('../../lib/tmp'); @@ -30,9 +25,8 @@ module.exports = function () { } } - 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); + if (sigintListeners.length > 1) this.fail('EEXISTS:MULTIPLE: existing SIGINT listener was not removed', this.exit); + if (sigintListeners.length != 1) this.fail('ENOAVAIL: no SIGINT listener was installed', this.exit); + // tmp will now exit the process as there are no custom user listeners installed + sigintListeners[0](); }; diff --git a/test/outband/issue129.js b/test/outband/issue129.js index a36c56b..311704e 100644 --- a/test/outband/issue129.js +++ b/test/outband/issue129.js @@ -8,7 +8,7 @@ module.exports = function () { return (listener.name === '_exit' || listener.name === '_uncaughtExceptionThrown') && listener.toString().indexOf('_garbageCollector();') !== -1; } -å + function _garbageCollector() {} var callState = { @@ -63,7 +63,7 @@ module.exports = function () { 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);