test: fix test-debug-port-numbers on OS X#7046
Conversation
|
This test was failing in |
|
Stress test without this change fails: https://ci.nodejs.org/job/node-stress-single-test/749. |
There was a problem hiding this comment.
I think the logic should be ESRCH || EPERM iff OS X, and you might as well drop the 'iff OS X' part in that case because the BSDs probably exhibit the same behavior.
I checked the xnu and libc sources and the error code seems to depend on whether POSIX compatibility mode is enabled in the libc wrapper: it returns EPERM if it is, ESRCH otherwise. That would explain why I don't see EPERM with 10.8.
|
LGTM. I think @bnoordhuis suggestion makes sense. One question though:
Does that mean that processes may be left behind? |
|
@cjihrig My reading of the xnu sources is that EPERM or ESRCH are only returned when the process group is gone (i.e., empty.) |
|
@bnoordhuis, from the |
|
I think xnu inherits most of its signal handling logic from FreeBSD but it's possible that the POSIX compatibility stuff is a later addition. I don't think allowing EPERM on other platforms will hurt, at any rate. |
|
Yes, it makes sense. So just to be sure... |
|
I'd do it unconditionally, i.e., no platform-specific checks. |
|
PR updated per your comments. Thanks! |
There was a problem hiding this comment.
I don't think this is right. 'EPERM' || 'ESRCH' will always evaluate to EPERM
There was a problem hiding this comment.
Also, assert.ok() just asserts that e.code is truthy.
There was a problem hiding this comment.
You're right. I was fixing that :(
|
Updated |
|
LGTM |
|
LGTM |
According to kill(2), kill returns `EPERM` error if when signalling a process group any of the members could not be signalled. PR-URL: nodejs#7046 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
CI: https://ci.nodejs.org/job/node-test-commit/3579/. All green except some unrelated failures in some ARM bots. Landing |
|
Landed in 6e148a3. Thanks! |
According to kill(2), kill returns `EPERM` error if when signalling a process group any of the members could not be signalled. PR-URL: nodejs#7046 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
According to kill(2), kill returns `EPERM` error if when signalling a process group any of the members could not be signalled. PR-URL: #7046 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
According to kill(2), kill returns `EPERM` error if when signalling a process group any of the members could not be signalled. PR-URL: #7046 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
According to kill(2), kill returns `EPERM` error if when signalling a process group any of the members could not be signalled. PR-URL: #7046 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
According to kill(2), kill returns `EPERM` error if when signalling a process group any of the members could not be signalled. PR-URL: #7046 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
According to kill(2), kill returns `EPERM` error if when signalling a process group any of the members could not be signalled. PR-URL: #7046 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
According to kill(2), kill returns `EPERM` error if when signalling a process group any of the members could not be signalled. PR-URL: #7046 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Checklist
Affected core subsystem(s)
test
Description of change
According to kill(2), kill returns
EPERMerror if when signalling aprocess group any of the members could not be signaled.
Refs: #7037