-
Notifications
You must be signed in to change notification settings - Fork 7.3k
process: pid can be a string in process.kill() #8531
Conversation
@@ -641,8 +641,8 @@ | |||
process.kill = function(pid, sig) { | |||
var err; | |||
|
|||
if (typeof pid !== 'number' || !isFinite(pid)) { | |||
throw new TypeError('pid must be a number'); | |||
if (pid != (pid | 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer pid >>> 0
. That is the only way to do uint
coercion, and since you can't have a negative pid makes sense to do it this way. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can send signals to negative pids (see man 2 kill), and negative numbers are tested for in the test, and supported by node/libuv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, whoops. I'm thinking of fd's. sorry. :)
@sam-github one more small nit. Thanks |
@trevnorris ping |
Thanks. Merged in 743a009. |
Not allowing string was a change from v0.10 behaviour, commented on in #7991. Allow them again, but still check that argument is numberish. Also, simplify the fragile and non-portable test code introduced in 832ec1c that required fixups 2a41535, and ef3c4ed. PR-URL: #8531 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Thanks, @trevnorris |
Not allowing string was a change from v0.10 behaviour, commented on in
#7991. Allow them again, but still check that argument is
numberish. Also, simplify the fragile and non-portable test code
introduced in 832ec1c that required fixups 2a41535, and
ef3c4ed.
Sorry about the test awfulness I introduced, @misterdjules @tjfontaine.