Skip to content

Commit ae61bb6

Browse files
silkentrancesilkentrance
authored
Merge pull request #193 from raszi/gh-192
fix #192: tmp must not exit the process on its own
2 parents 05aba23 + 9275e3e commit ae61bb6

File tree

7 files changed

+64
-84
lines changed

7 files changed

+64
-84
lines changed

‎lib/tmp.js

Lines changed: 28 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ const
4141

4242
SIGINT = 'SIGINT',
4343

44+
SIGINT_LISTENER_NAME = '_tmp$sigint_listener'
45+
4446
// this will hold the objects need to be removed on exit
4547
_removeObjects = [];
4648

@@ -608,33 +610,43 @@ function _is_legacy_listener(listener) {
608610
*/
609611
function _safely_install_sigint_listener() {
610612

611-
const listeners = process.listeners(SIGINT);
613+
let listeners = process.listeners(SIGINT);
612614
const existingListeners = [];
613615
for (let i = 0, length = listeners.length; i < length; i++) {
614616
const lstnr = listeners[i];
615617
/* istanbul ignore else */
616-
if (lstnr.name === '_tmp$sigint_listener') {
618+
if (lstnr.name === SIGINT_LISTENER_NAME) {
617619
existingListeners.push(lstnr);
618620
process.removeListener(SIGINT, lstnr);
619621
}
620622
}
621-
process.on(SIGINT, function _tmp$sigint_listener(doExit) {
623+
process.on(SIGINT, function _tmp$sigint_listener() {
622624
for (let i = 0, length = existingListeners.length; i < length; i++) {
623-
// let the existing listener do the garbage collection (e.g. jest sandbox)
624625
try {
625-
existingListeners[i](false);
626+
// let the existing listener do the garbage collection (e.g. jest sandbox)
627+
// pass in false in case that we are dealing with an old version of this
628+
existingListeners[i](false);
626629
} catch (err) {
627630
// ignore
628631
}
629632
}
630-
try {
631-
// force the garbage collector even it is called again in the exit listener
632-
_garbageCollector();
633-
} finally {
634-
if (!!doExit) {
635-
process.exit(0);
636-
}
637-
}
633+
// check if there are any user installed sigint listeners
634+
// if there are none, exit the process
635+
let doExit = true;
636+
listeners = process.listeners(SIGINT);
637+
for (let i = 0, length = listeners.length; i < length; i++) {
638+
const lstnr = listeners[i];
639+
if (lstnr.name !== SIGINT_LISTENER_NAME) {
640+
doExit = false;
641+
break;
642+
}
643+
}
644+
// force the garbage collector even it is called again in the exit listener
645+
_garbageCollector();
646+
// exit the process if no user listeners have been installed
647+
if (doExit) {
648+
process.exit();
649+
}
638650
});
639651
}
640652

@@ -660,12 +672,11 @@ function _safely_install_exit_listener() {
660672
process.removeListener(EXIT, lstnr);
661673
}
662674
}
663-
// TODO: what was the data parameter good for?
664-
process.addListener(EXIT, function _tmp$safe_listener(data) {
675+
process.addListener(EXIT, function _tmp$safe_listener(code) {
665676
for (let i = 0, length = existingListeners.length; i < length; i++) {
666-
// let the existing listener do the garbage collection (e.g. jest sandbox)
667677
try {
668-
existingListeners[i](data);
678+
// let the existing listener do the garbage collection (e.g. jest sandbox)
679+
existingListeners[i](code);
669680
} catch (err) {
670681
// ignore
671682
}

‎test/issue121-test.js

Lines changed: 8 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ const
77
os = require('os'),
88
rimraf = require('rimraf'),
99
testCases = [
10-
{ signal: 'SIGINT', expectExists: false },
11-
{ signal: 'SIGTERM', expectExists: true }
10+
'SIGINT',
11+
'SIGTERM'
1212
];
1313

1414
// skip tests on win32
@@ -18,10 +18,10 @@ const tfunc = isWindows ? xit : it;
1818
describe('tmp', function () {
1919
describe('issue121 - clean up on terminating signals', function () {
2020
for (let tc of testCases) {
21-
tfunc('for signal ' + tc.signal, function (done) {
22-
// increase timeout so that the child process may fail
23-
this.timeout(20000);
24-
issue121Tests(tc.signal, tc.expectExists)(done);
21+
tfunc('for signal ' + tc, function (done) {
22+
// increase timeout so that the child process may terminate in time
23+
this.timeout(5000);
24+
issue121Tests(tc)(done);
2525
});
2626
}
2727
});
@@ -33,22 +33,8 @@ function issue121Tests(signal, expectExists) {
3333
if (err) return done(err);
3434
else if (stderr) return done(new Error(stderr));
3535

36-
try {
37-
if (expectExists) {
38-
assertions.assertExists(stdout);
39-
}
40-
else {
41-
assertions.assertDoesNotExist(stdout);
42-
}
43-
done();
44-
} catch (err) {
45-
done(err);
46-
} finally {
47-
// cleanup
48-
if (expectExists) {
49-
rimraf.sync(stdout);
50-
}
51-
}
36+
assertions.assertDoesNotExist(stdout);
37+
done();
5238
}, signal);
5339
};
5440
}

‎test/issue129-sigint-test.js

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,23 +11,16 @@ describe('tmp', function () {
1111
it('when simulating sandboxed behavior', function (done) {
1212
childProcess(this, 'issue129-sigint.json', function (err, stderr, stdout) {
1313
if (err) return done(err);
14-
if (!stdout && !stderr) return done(new Error('stderr or stdout expected'));
1514
if (stderr) {
16-
try {
17-
assertions.assertDoesNotStartWith(stderr, 'EEXISTS:MULTIPLE');
18-
assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:EXISTING');
19-
} catch (err) {
20-
return done(err);
21-
}
15+
assertions.assertDoesNotStartWith(stderr, 'EEXISTS:MULTIPLE');
16+
assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:');
17+
return done();
2218
}
2319
if (stdout) {
24-
try {
25-
assert.equal(stdout, 'EOK', 'existing listeners should have been removed and called');
26-
} catch (err) {
27-
return done(err);
28-
}
20+
assert.equal(stdout, 'EOK');
21+
return done();
2922
}
30-
done();
23+
done(new Error('existing listener has not been called'));
3124
});
3225
});
3326
});

‎test/issue129-test.js

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,23 +13,15 @@ describe('tmp', function () {
1313
if (err) return done(err);
1414
if (!stdout && !stderr) return done(new Error('stderr or stdout expected'));
1515
if (stderr) {
16-
try {
17-
assertions.assertDoesNotStartWith(stderr, 'EEXISTS:LEGACY:EXIT');
18-
assertions.assertDoesNotStartWith(stderr, 'EEXISTS:LEGACY:UNCAUGHT');
19-
assertions.assertDoesNotStartWith(stderr, 'EEXISTS:NEWSTYLE');
20-
assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:LEGACY:EXIT');
21-
assertions.assertDoesNotStartWith(stderr, 'EAVAIL:LEGACY:UNCAUGHT');
22-
assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:NEWSTYLE');
23-
} catch (err) {
24-
return done(err);
25-
}
16+
assertions.assertDoesNotStartWith(stderr, 'EEXISTS:LEGACY:EXIT');
17+
assertions.assertDoesNotStartWith(stderr, 'EEXISTS:LEGACY:UNCAUGHT');
18+
assertions.assertDoesNotStartWith(stderr, 'EEXISTS:NEWSTYLE');
19+
assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:LEGACY:EXIT');
20+
assertions.assertDoesNotStartWith(stderr, 'EAVAIL:LEGACY:UNCAUGHT');
21+
assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:NEWSTYLE');
2622
}
2723
if (stdout) {
28-
try {
29-
assert.equal(stdout, 'EOK', 'existing listeners should have been removed and called');
30-
} catch (err) {
31-
return done(err);
32-
}
24+
assert.equal(stdout, 'EOK', 'existing listeners should have been removed and called');
3325
}
3426
done();
3527
});

‎test/outband/issue121.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@
44
const
55
tmp = require('../../lib/tmp');
66

7+
process.on('SIGTERM', function () {
8+
process.exit(0);
9+
});
10+
711
// https://github.com/raszi/node-tmp/issues/121
812
module.exports = function () {
913

‎test/outband/issue129-sigint.js

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,13 @@
44
// addendum to https://github.com/raszi/node-tmp/issues/129 so that with jest sandboxing we do not install our sigint
55
// listener multiple times
66
module.exports = function () {
7-
var callState = {
8-
existingListener : false,
9-
};
107

11-
// simulate an existing SIGINT listener
12-
var listener1 = (function (callState) {
13-
return function _tmp$sigint_listener(doExit) {
14-
callState.existingListener = !doExit;
15-
};
16-
})(callState);
8+
var self = this;
179

18-
process.addListener('SIGINT', listener1);
10+
// simulate an existing SIGINT listener
11+
process.addListener('SIGINT', function _tmp$sigint_listener() {
12+
self.out('EOK');
13+
});
1914

2015
// now let tmp install its listener safely
2116
require('../../lib/tmp');
@@ -30,9 +25,8 @@ module.exports = function () {
3025
}
3126
}
3227

33-
if (listeners.length > 1) this.fail('EEXISTS:MULTIPLE: existing SIGINT listener was not removed', this.exit);
34-
listeners[0](false); // prevent listener from exiting the process
35-
if (!callState.existingListener) this.fail('ENOAVAIL:EXISTING: existing listener was not called', this.exit);
36-
this.out('EOK', this.exit);
37-
process.exit(0);
28+
if (sigintListeners.length > 1) this.fail('EEXISTS:MULTIPLE: existing SIGINT listener was not removed', this.exit);
29+
if (sigintListeners.length != 1) this.fail('ENOAVAIL: no SIGINT listener was installed', this.exit);
30+
// tmp will now exit the process as there are no custom user listeners installed
31+
sigintListeners[0]();
3832
};

‎test/outband/issue129.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ module.exports = function () {
88
return (listener.name === '_exit' || listener.name === '_uncaughtExceptionThrown')
99
&& listener.toString().indexOf('_garbageCollector();') !== -1;
1010
}
11-
å
11+
1212
function _garbageCollector() {}
1313

1414
var callState = {
@@ -63,7 +63,7 @@ module.exports = function () {
6363
if (listener.name === '_uncaughtExceptionThrown') legacyUncaughtListener = listener;
6464
else legacyExitListener = listener;
6565
}
66-
}å
66+
}
6767

6868
if (legacyExitListener) this.fail('EEXISTS:LEGACY:EXIT existing legacy exit listener was not removed', this.exit);
6969
if (legacyUncaughtListener) this.fail('EEXISTS:LEGACY:UNCAUGHT existing legacy uncaught exception thrown listener was not removed', this.exit);

0 commit comments

Comments
 (0)