Skip to content

child_process: fix channel disconnect logic #19566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

child_process: fix channel disconnect logic #19566

wants to merge 1 commit into from

Conversation

laino
Copy link

@laino laino commented Mar 23, 2018 •
edited
Loading

  • Fix close not being emitted when calling process.disconnect()
    from a parent process.
  • Extend child-process-disconnect test case to also check that 'close'
    was emitted after 'exit' after 'disconnect'.
  • Create child-process-close-handle-queue test case, checking
    that 'send' callbacks of pending handles are called.

Fixes: #19433

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

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Mar 23, 2018
@laino laino force-pushed the child-process-close branch from 0943fd3 to 49c68b9 Compare March 23, 2018 20:03
@jasnell jasnell requested review from addaleax and bnoordhuis March 23, 2018 20:06
@laino
Copy link
Author

laino commented Mar 23, 2018

So running the new test against the old code I realized closing the handle when an error is reported is actually inconsistent with the previous behavior. Also in the queue there aren't actually handles, but the server/socket/etc. objects as passed by the user. Oops.

I'll update the PR to fix that in second.

@laino laino force-pushed the child-process-close branch 3 times, most recently from c596738 to fff9957 Compare March 24, 2018 04:39
@laino laino changed the title child_process: fix channel disconnect logic & cleanup pending handles child_process: fix channel disconnect logic Mar 24, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

@nodejs/child_process @addaleax @bnoordhuis PTAL

@BridgeAR
Copy link
Member

Ping @nodejs/collaborators @nodejs/tsc this needs some people who worked on child_process before to have a look at it. It is open for a long time without a single comment so far...

tniessen
tniessen reviewed May 2, 2018
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't really comment on the change itself, sorry.

bnoordhuis
bnoordhuis approved these changes May 2, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments but apart from that LGTM.

lib/internal/child_process.js Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this into the if block? Callbacks and event listeners should get fresh exception objects so they don't step on each other when they modify it.

test/parallel/test-child-process-disconnect.js Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly nicer to set socket.setEncoding('utf8') once.

@fhinkel
Copy link
Member

fhinkel commented Jun 5, 2018

@laino Do you want to look into the requested changes?

@ryzokuken
Copy link
Contributor

ping @laino

@maclover7
Copy link
Contributor

ping @laino

@maclover7 maclover7 added the wip Issues and PRs that are still a work in progress. label Aug 11, 2018
@laino laino force-pushed the child-process-close branch from fff9957 to 74ceb62 Compare September 8, 2018 15:21
@laino
Copy link
Author

laino commented Sep 8, 2018

Implemented the requested changes and rebased onto the current master.

@lundibundi
Copy link
Member

Cannot comment a change, unfortunately (I'm not really familiar with child_process) though @laino could you rebase this once again and I'll then start a CI for this one?

lundibundi
lundibundi reviewed Oct 1, 2018
View reviewed changes
test/parallel/test-child-process-close-handle-queue.js Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I don't think additional parentheses are needed here.

test/parallel/test-child-process-close-handle-queue.js Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert.ifError( ?

@lundibundi
Copy link
Member

ping @laino

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Oct 16, 2018
@laino @Trott
child_process: fix channel disconnect logic
40a840d 
- Fix close not being emitted when calling process.disconnect()
  from a parent process.
- Extend child-process-disconnect test case to also check that 'close'
  was emitted after 'exit' after 'disconnect'.
- Create child-process-close-handle-queue test case, checking
  that 'send' callbacks of pending handles are called.

Fixes: nodejs#19433
@Trott Trott force-pushed the child-process-close branch from 74ceb62 to 40a840d Compare November 10, 2018 23:59
@Trott
Copy link
Member

Trott commented Nov 11, 2018

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

@fhinkel
Copy link
Member

fhinkel commented Oct 26, 2019

Closing this due to inactivity. Please reopen if needed.

@fhinkel fhinkel closed this Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments

@tniessen tniessen tniessen left review comments

@BridgeAR BridgeAR BridgeAR left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax Awaiting requested review from addaleax

+1 more reviewer

@lundibundi lundibundi lundibundi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem. stalled Issues and PRs that are stalled. wip Issues and PRs that are still a work in progress.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

child_process: 'close' not emitted after .disconnect() in parent process