test: fix test-debug-port-numbers on OS X #7046

santigimeno · 2016-05-29T11:00:56Z

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

According to kill(2), kill returns EPERM error if when signalling a
process group any of the members could not be signaled.

Refs: #7037

santigimeno · 2016-05-29T11:03:06Z

This test was failing in OS X https://ci.nodejs.org/job/node-test-commit-osx/3581/nodes=osx1010/:

not ok 191 parallel/test-debug-port-numbers
# 
# assert.js:90
#   throw new assert.AssertionError({
#   ^
# AssertionError: 'EPERM' === 'ESRCH'
#     at kill (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-debug-port-numbers.js:47:12)
#     at update (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-debug-port-numbers.js:37:7)
#     at Socket.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-debug-port-numbers.js:21:65)
#     at emitOne (events.js:101:20)
#     at Socket.emit (events.js:188:7)
#     at readableAddChunk (_stream_readable.js:172:18)
#     at Socket.Readable.push (_stream_readable.js:130:10)
#     at Pipe.onread (net.js:542:20)
# debug> debug> debug> debug> �< Debugger listening on port 12347
# debug> �connecting to 127.0.0.1:12347 ... ok
# debug> �< Debugger listening on port 12348
# debug> �connecting to 127.0.0.1:12348 ...�< Debugger listening on port 12346
# debug> �connecting to 127.0.0.1:12346 ...�< Debugger listening on port 12349
# debug> �connecting to 127.0.0.1:12349 ... ok
# debug>  ok
# debug>  ok
# debug> �break in test/parallel/test-debug-port-numbers.js:1
# �> 1 'use strict';
# �  2 
# �  3 const common = require('../common');
# debug> �break in test/parallel/test-debug-port-numbers.js:1
# �> 1 'use strict';
# �  2 
# �  3 const common = require('../common');
# debug> �break in test/parallel/test-debug-port-numbers.js:1
# �> 1 'use strict';
# �  2 
# �  3 const common = require('../common');
# �break in test/parallel/test-debug-port-numbers.js:1

santigimeno · 2016-05-29T11:24:32Z

Stress test without this change fails: https://ci.nodejs.org/job/node-stress-single-test/749.
Stress test with this change passes: https://ci.nodejs.org/job/node-stress-single-test/750.

bnoordhuis · 2016-05-29T13:01:12Z

test/parallel/test-debug-port-numbers.js

+    if (process.platform === 'darwin')
+      assert.strictEqual(e.code, 'EPERM');  // Already gone.
+    else
+      assert.strictEqual(e.code, 'ESRCH');  // Already gone.


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.

cjihrig · 2016-05-29T13:04:06Z

LGTM. I think @bnoordhuis suggestion makes sense. One question though:

According to kill(2), kill returns EPERM error if when signalling a process group any of the members could not be signaled.

Does that mean that processes may be left behind?

bnoordhuis · 2016-05-29T13:08:45Z

@cjihrig My reading of the xnu sources is that EPERM or ESRCH are only returned when the process group is gone (i.e., empty.)

santigimeno · 2016-05-29T13:10:49Z

@bnoordhuis, from the FreeBSD 10.3 man: [EPERM] The sending process does not have permission to send sig to the receiving process. So I'm not sure if it applies to FreeBSD. I can add it anyway.

bnoordhuis · 2016-05-29T13:13:03Z

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.

santigimeno · 2016-05-29T13:15:34Z

Yes, it makes sense. So just to be sure... ESRCH || EPERM on OS X and FreeBSD right?

bnoordhuis · 2016-05-29T13:17:58Z

I'd do it unconditionally, i.e., no platform-specific checks.

santigimeno · 2016-05-29T13:25:26Z

PR updated per your comments. Thanks!

santigimeno · 2016-05-29T13:26:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/2847/

cjihrig · 2016-05-29T13:44:45Z

test/parallel/test-debug-port-numbers.js

-    assert.strictEqual(e.code, 'ESRCH');  // Already gone.
+    // Generally ESRCH is returned when the process group is already gone. On
+    // some platforms such as OS X it may be EPERM though.
+    assert.ok(e.code, 'EPERM' || 'ESRCH');


I don't think this is right. 'EPERM' || 'ESRCH' will always evaluate to EPERM

Also, assert.ok() just asserts that e.code is truthy.

You're right. I was fixing that :(

santigimeno · 2016-05-29T14:00:18Z

Updated

cjihrig · 2016-05-29T14:02:23Z

LGTM

santigimeno · 2016-05-29T14:02:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/2848/

bnoordhuis · 2016-05-29T14:52:49Z

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>

santigimeno · 2016-05-30T08:20:40Z

CI: https://ci.nodejs.org/job/node-test-commit/3579/. All green except some unrelated failures in some ARM bots. Landing

santigimeno · 2016-05-30T08:22:37Z

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>

nodejs-github-bot added the test Issues and PRs related to the tests. label May 29, 2016

santigimeno added the debugger label May 29, 2016

bnoordhuis reviewed May 29, 2016
View reviewed changes

santigimeno force-pushed the debug_port branch from 6ae9ff3 to 8bfae19 Compare May 29, 2016 13:24

cjihrig reviewed May 29, 2016
View reviewed changes

santigimeno force-pushed the debug_port branch from 8bfae19 to 9959a71 Compare May 29, 2016 13:53

mscdex added the macos Issues and PRs related to the macOS platform / OSX. label May 29, 2016

test: fix test-debug-port-numbers on OS X

6e148a3

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>

santigimeno force-pushed the debug_port branch from 9959a71 to 6e148a3 Compare May 30, 2016 07:41

santigimeno merged commit 6e148a3 into nodejs:master May 30, 2016

santigimeno deleted the debug_port branch May 30, 2016 08:21

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the land-on-v4.x label Jul 11, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix test-debug-port-numbers on OS X #7046

test: fix test-debug-port-numbers on OS X #7046

santigimeno commented May 29, 2016 •

edited

Loading

santigimeno commented May 29, 2016

santigimeno commented May 29, 2016

bnoordhuis May 29, 2016

cjihrig commented May 29, 2016

bnoordhuis commented May 29, 2016

santigimeno commented May 29, 2016

bnoordhuis commented May 29, 2016

santigimeno commented May 29, 2016

bnoordhuis commented May 29, 2016

santigimeno commented May 29, 2016

santigimeno commented May 29, 2016

cjihrig May 29, 2016

cjihrig May 29, 2016

santigimeno May 29, 2016

santigimeno commented May 29, 2016

cjihrig commented May 29, 2016

santigimeno commented May 29, 2016

bnoordhuis commented May 29, 2016

santigimeno commented May 30, 2016

santigimeno commented May 30, 2016

test: fix test-debug-port-numbers on OS X #7046

test: fix test-debug-port-numbers on OS X #7046

Conversation

santigimeno commented May 29, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

santigimeno commented May 29, 2016

santigimeno commented May 29, 2016

bnoordhuis May 29, 2016

Choose a reason for hiding this comment

cjihrig commented May 29, 2016

bnoordhuis commented May 29, 2016

santigimeno commented May 29, 2016

bnoordhuis commented May 29, 2016

santigimeno commented May 29, 2016

bnoordhuis commented May 29, 2016

santigimeno commented May 29, 2016

santigimeno commented May 29, 2016

cjihrig May 29, 2016

Choose a reason for hiding this comment

cjihrig May 29, 2016

Choose a reason for hiding this comment

santigimeno May 29, 2016

Choose a reason for hiding this comment

santigimeno commented May 29, 2016

cjihrig commented May 29, 2016

santigimeno commented May 29, 2016

bnoordhuis commented May 29, 2016

santigimeno commented May 30, 2016

santigimeno commented May 30, 2016

santigimeno commented May 29, 2016 •

edited

Loading