child_process: workaround fd passing issue on OS X by santigimeno · Pull Request #7572 · nodejs/node

santigimeno · 2016-07-06T21:54:33Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

child_process

Description of change

There's an issue on some OS X versions when passing fd's between processes.
When the handle associated to a specific file descriptor is closed by the sender
process before it's received in the destination, the handle is indeed closed
while it should remain opened. As a workaround for OS X, don't close the
handle until the NODE_HANDLE_ACK is received by the sender.
Added test-child-process-pass-fd that is basically test-cluster-net-send but
creating lots of workers, so the issue reproduces on OS X consistently.

Fixes: #7512

/cc @bnoordhuis

Fishrock123 · 2016-07-07T08:33:53Z

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

cjihrig · 2016-07-07T16:05:01Z

Would there be any harm in dropping the darwin checks here and in the tests?

FWIW, I also think it's better not to special-case.

santigimeno · 2016-07-07T20:28:41Z

PR updated addressing @bnoordhuis and @cjihrig comments. Also, it includes modifications to make sure the _pendingHandle is closed even if the NODE_HANDLE_ACK is not received due to IPC channel disconnection. Thanks

bnoordhuis · 2016-07-08T08:34:55Z

Should target._pendingHandle be a list or a Set? ISTM that it's not inconceivable for another handle to be sent while still waiting for the ACK for the first handle.

I thought that was not possible because the messages/handles where stored in _handleQueue while there were pending ACK's: https://github.com/nodejs/node/blob/master/lib/internal/child_process.js#L566-575. Is this assumption correct?

Oh, you're right. That's a bit of an implicit relationship, though. Maybe you can add a comment explaining the dependency on _handleQueue?

bnoordhuis · 2016-07-09T09:39:43Z

LGTM with a comment.

santigimeno · 2016-07-10T16:22:50Z

Added requested comment. Not too sure of the wording though.

bnoordhuis · 2016-07-11T09:01:41Z

LGTM

Added requested comment. Not too sure of the wording though.

It's fine. The next person knows they'll need to look at _handleQueue and that's the important thing.

cjihrig · 2016-07-11T12:53:59Z

Can you use strictEqual() here and throughout.

cjihrig · 2016-07-11T12:56:11Z

LGTM with a couple comments.

santigimeno · 2016-07-12T07:36:20Z

Test updated. Rebased and squashed into a single commit.

bnoordhuis · 2016-07-12T07:49:37Z

I don't know if it's a good idea to spawn 80 processes. You might hit the ulimit on some systems and it will be very slow on the less powerful buildbots. It takes about 500 ms to start a process on the rpi1.

I understand. That was one of the reasons why initially I was running this test only on OS X. I chose 80 because it was the magic number that caused the test to fail in my OS X almost 100% of the time. Maybe it's not a bad idea to run the test only in OS X as it seems an OS X only bug, and the basic functionality the test is checking is already covered by test-cluster-net-send.

I suppose you could skip if '67'.includes(process.config.variables.arm_version), that would filter out the ARMv6 and ARMv7 buildbots (i.e., the rpis.)

santigimeno · 2016-07-13T19:49:47Z

Updated so it skips the test in armv6 and armv7.
CI: https://ci.nodejs.org/job/node-test-pull-request/3285/

santigimeno · 2016-07-14T08:07:14Z

Unfortunately this change breaks test-cluster-send-handle-twice on win10. I'll try to look into it.

santigimeno · 2016-07-26T21:01:46Z

Strangely, the problem in win10 was that the NODE_HANDLE_ACK was received before the req.oncomplete was called which in turn calls the postSend method. Calling the postSend method just after sending the handle through the IPC seems to fix the issue (not completely sure it's the correct solution though).

santigimeno · 2016-07-26T21:04:40Z

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

santigimeno · 2016-08-17T20:14:37Z

@bnoordhuis @cjihrig Thoughts?

bnoordhuis · 2016-08-17T22:09:52Z

@santigimeno Are you asking for a re-review or is there an open question we should answer?

santigimeno · 2016-08-17T22:22:05Z

@bnoordhuis basically a review. I'm not sure it's a good idea not waiting for req.oncomplete to call postSend though, but that's the only way I could find to fix the win10 issue.

santigimeno · 2016-08-18T17:00:15Z

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

santigimeno · 2016-08-18T18:42:09Z

Incorporated @cjihrig suggestion and squashed into a single commit. Last CI was green. I'll land this tomorrow if there's no objection.

santigimeno · 2016-08-19T14:34:30Z

One last CI: https://ci.nodejs.org/job/node-test-pull-request/3750/

santigimeno · 2016-08-19T15:49:55Z

One more CI run as there were some problems in the arm bots:
https://ci.nodejs.org/job/node-test-pull-request/3752/

There's an issue on some `OS X` versions when passing fd's between processes. When the handle associated to a specific file descriptor is closed by the sender process before it's received in the destination, the handle is indeed closed while it should remain opened. In order to fix this behaviour, don't close the handle until the `NODE_HANDLE_ACK` is received by the sender. Added `test-child-process-pass-fd` that is basically `test-cluster-net-send` but creating lots of workers, so the issue reproduces on `OS X` consistently. Fixes: nodejs#7512 PR-URL: nodejs#7572 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

santigimeno · 2016-08-20T10:24:39Z

Last CI: https://ci.nodejs.org/job/node-test-pull-request/3765/
All is green except for test-crypto-timing-safe-equal failures.

santigimeno · 2016-08-20T10:26:09Z

Landed in db6253f. Thanks!

There's an issue on some `OS X` versions when passing fd's between processes. When the handle associated to a specific file descriptor is closed by the sender process before it's received in the destination, the handle is indeed closed while it should remain opened. In order to fix this behaviour, don't close the handle until the `NODE_HANDLE_ACK` is received by the sender. Added `test-child-process-pass-fd` that is basically `test-cluster-net-send` but creating lots of workers, so the issue reproduces on `OS X` consistently. Fixes: #7512 PR-URL: #7572 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2016-09-30T19:02:51Z

@santigimeno / @bnoordhuis / @cjihrig should this be backported?

santigimeno · 2016-10-03T08:31:06Z

@thealphanerd I think so. Do you need a backport PR?

MylesBorins · 2016-10-03T08:31:44Z

yes please

There's an issue on some `OS X` versions when passing fd's between processes. When the handle associated to a specific file descriptor is closed by the sender process before it's received in the destination, the handle is indeed closed while it should remain opened. In order to fix this behaviour, don't close the handle until the `NODE_HANDLE_ACK` is received by the sender. Added `test-child-process-pass-fd` that is basically `test-cluster-net-send` but creating lots of workers, so the issue reproduces on `OS X` consistently. Fixes: nodejs#7512 Ref: nodejs#7572 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

santigimeno · 2016-10-03T10:48:37Z

@thealphanerd backport here: #8904

There's an issue on some `OS X` versions when passing fd's between processes. When the handle associated to a specific file descriptor is closed by the sender process before it's received in the destination, the handle is indeed closed while it should remain opened. In order to fix this behaviour, don't close the handle until the `NODE_HANDLE_ACK` is received by the sender. Added `test-child-process-pass-fd` that is basically `test-cluster-net-send` but creating lots of workers, so the issue reproduces on `OS X` consistently. Fixes: #7512 Ref: #8904 PR-URL: #7572 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

santigimeno added child_process Issues and PRs related to the child_process subsystem. macos Issues and PRs related to the macOS platform / OSX. labels Jul 6, 2016

cjihrig reviewed Jul 7, 2016
View reviewed changes

santigimeno force-pushed the pass_fd branch from a5cd002 to b3907beCompare July 7, 2016 20:20

bnoordhuis reviewed Jul 8, 2016
View reviewed changes

santigimeno force-pushed the pass_fd branch from 698f534 to 74d5b3eCompare July 10, 2016 16:21

cjihrig reviewed Jul 11, 2016
View reviewed changes

Comment threadtest/sequential/test-child-process-pass-fd.js Outdated
cjihrigJul 11, 2016
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use strictEqual() here and throughout.

santigimeno force-pushed the pass_fd branch from 74d5b3e to 25c4c2bCompare July 12, 2016 07:35

bnoordhuis reviewed Jul 12, 2016
View reviewed changes

santigimeno force-pushed the pass_fd branch from 25c4c2b to 99ed72aCompare July 13, 2016 19:30

santigimeno force-pushed the pass_fd branch from c499c04 to 67ba433Compare July 13, 2016 22:03

santigimeno force-pushed the pass_fd branch from 67ba433 to a3f5d50Compare July 26, 2016 20:56

santigimeno force-pushed the pass_fd branch from a3f5d50 to b44b1c8Compare August 18, 2016 16:59

santigimeno force-pushed the pass_fd branch from b44b1c8 to 8c238adCompare August 18, 2016 18:39

santigimeno force-pushed the pass_fd branch from 8c238ad to bc7bcbbCompare August 19, 2016 14:30

santigimeno force-pushed the pass_fd branch from bc7bcbb to db6253fCompare August 20, 2016 08:59

santigimeno merged commit db6253f into nodejs:masterAug 20, 2016

evanlucas mentioned this pull request Aug 24, 2016
2016-08-26, Version 6.5.0 (Current) #8253
Merged

MylesBorins added the lts-watch-v4.x label Sep 30, 2016

santigimeno mentioned this pull request Oct 3, 2016
Backport PR 7572 #8904
Closed
4 tasks

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Oct 4, 2016

MylesBorins mentioned this pull request Oct 26, 2016
V4.6.2 proposal #9298
Closed

Uh oh!

Uh oh!

Conversation

santigimeno commented Jul 6, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123 commented Jul 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

santigimeno commented Jul 7, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jul 9, 2016

Uh oh!

santigimeno commented Jul 10, 2016

Uh oh!

bnoordhuis commented Jul 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Jul 11, 2016

Uh oh!

santigimeno commented Jul 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

santigimeno commented Jul 13, 2016

Uh oh!

santigimeno commented Jul 14, 2016

Uh oh!

santigimeno commented Jul 26, 2016

Uh oh!

santigimeno commented Jul 26, 2016

Uh oh!

santigimeno commented Aug 17, 2016

Uh oh!

bnoordhuis commented Aug 17, 2016

Uh oh!

santigimeno commented Aug 17, 2016

Uh oh!

santigimeno commented Aug 18, 2016

Uh oh!

santigimeno commented Aug 18, 2016

Uh oh!

santigimeno commented Aug 19, 2016

Uh oh!

santigimeno commented Aug 19, 2016

Uh oh!

santigimeno commented Aug 20, 2016

Uh oh!

santigimeno commented Aug 20, 2016

Uh oh!

MylesBorins commented Sep 30, 2016

Uh oh!

santigimeno commented Oct 3, 2016

Uh oh!

MylesBorins commented Oct 3, 2016

Uh oh!

santigimeno commented Oct 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

santigimeno commented Jul 7, 2016•
edited
Loading