Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #192: tmp must not exit the process on its own #193

Merged
merged 1 commit into from
Nov 15, 2019
Merged

fix #192: tmp must not exit the process on its own #193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 28 additions & 17 deletions lib/tmp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];

Expand Down Expand Up @@ -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();
}
});
}

Expand All @@ -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
}
Expand Down
30 changes: 8 additions & 22 deletions test/issue121-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
});
}
});
Expand All @@ -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);
};
}
19 changes: 6 additions & 13 deletions test/issue129-sigint-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
});
});
});
Expand Down
22 changes: 7 additions & 15 deletions test/issue129-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
4 changes: 4 additions & 0 deletions test/outband/issue121.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {

Expand Down
24 changes: 9 additions & 15 deletions test/outband/issue129-sigint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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]();
};
4 changes: 2 additions & 2 deletions test/outband/issue129.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module.exports = function () {
return (listener.name === '_exit' || listener.name === '_uncaughtExceptionThrown')
&& listener.toString().indexOf('_garbageCollector();') !== -1;
}
å

function _garbageCollector() {}

var callState = {
Expand Down Expand Up @@ -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);
Expand Down