fix #129 install process listeners safely#130
Conversation
0ad4041 to
26f778b
Compare
5f969f9 to
b838d3a
Compare
lib/tmp.js
Outdated
|
|
||
| // we use this for determining which SIGINT handler we need to install | ||
| // TODO:is M$ so dim, win32 for win64 and future win128 platforms? | ||
| const isWindows = process.platform == 'win32'; |
lib/tmp.js
Outdated
| if (_isUndefined(options)) { | ||
| return [{}, callback]; | ||
| if (typeof options == 'function') { | ||
| result[0] = {}; |
There was a problem hiding this comment.
I prefer the early returns and the immutable values instead of this.
lib/tmp.js
Outdated
| /* | ||
| * Module dependencies. | ||
| */ | ||
| const tmpdir = require('os-tmpdir'); |
There was a problem hiding this comment.
Why was it necessary to rename the const?
lib/tmp.js
Outdated
|
|
||
| for (var i = 0, length = listeners.length; i < length; i++) { | ||
| var lstnr = listeners[i]; | ||
| if ((lstnr.name == listener.name) || _is_legacy_listener(listener.name)) { |
There was a problem hiding this comment.
No need for the extra parens around the equality check.
lib/tmp.js
Outdated
| if (opts.template && !opts.template.match(TEMPLATE_PATTERN)) | ||
| return cb(new Error('Invalid template provided')); | ||
|
|
||
| // FIXME:get rid of duplicate code |
There was a problem hiding this comment.
Please put a space after the FIXME keyword, like this:
// FIXME get rid of duplicate codea8ec86d to
d851283
Compare
|
NOTE that this is still work in progress |
99df905 to
7aff3ad
Compare
lib/tmp.js
Outdated
| } | ||
|
|
||
| return [options, callback]; | ||
| return [options, callback]; |
There was a problem hiding this comment.
There is an unnecessary whitespace at the end of the line.
There was a problem hiding this comment.
fixed, found another one in line 571.
b8bbc57 to
243cc12
Compare
|
IMPORTANT tests are still missing! |
ac64a81 to
7c72786
Compare
|
There seems to be an issue with the new tests. I will have to look into that. |
6b9ffca to
5ea71f1
Compare
|
@raszi while the existing test case test/issue129-test and test/outband/issue129.js(on) does not completely encompass the behaviour of facebook/jest, which utilises sandboxes, I think that we can be safe in the assumption that tmp will register its handlers safely and not try to register handlers multiple times. That being said, I dropped the uncaughtException handler completely. And while earlier versions of tmp will still install that handler, I think that we can be safe to assume that it does not matter, as it gets collected by the function that safely installs the exit handler, making use of _is_legacy_listener(). In fact, any uncaught exception handlers installed by tmp will be removed altogether, leaving only the legacy _exit handlers. I tried to not be too smart about detecting whether it is a legacy handler installed by tmp, just limiting it to the handlers name and whether it includes a call to _garbageCollector();. However, we will still have to notify interested third parties to update their dependencies accordingly, especially when they are using facebook/jest or some other framework that makes use of sandboxes. |
See #129
WIP
TODO: additional tests