Skip to content

child_process: fix channel disconnect logic - #19566

Closed
laino wants to merge 1 commit into
nodejs:masterfrom
laino:child-process-close
Closed

child_process: fix channel disconnect logic#19566
laino wants to merge 1 commit into
nodejs:masterfrom
laino:child-process-close

Conversation

@laino

@lainolaino commented Mar 23, 2018•
edited
Loading

Copy link
Copy Markdown
  • 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-botnodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Mar 23, 2018
@laino
lainoforce-pushed the child-process-close branch from 0943fd3 to 49c68b9CompareMarch 23, 2018 20:03
@laino

laino commented Mar 23, 2018

Copy link
Copy Markdown
Author

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
lainoforce-pushed the child-process-close branch 3 times, most recently from c596738 to fff9957CompareMarch 24, 2018 04:39
@lainolaino changed the title child_process: fix channel disconnect logic & cleanup pending handleschild_process: fix channel disconnect logicMar 24, 2018
@BridgeAR

BridgeAR commented Apr 9, 2018

Copy link
Copy Markdown
Member

@nodejs/child_process @addaleax@bnoordhuis PTAL

@BridgeAR

BridgeAR commented Apr 28, 2018

Copy link
Copy Markdown
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

@tniessentniessen left a comment

Copy link
Copy Markdown
Member

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.

Comment threadlib/internal/child_process.js Outdated
bnoordhuis
bnoordhuis approved these changes May 2, 2018
View reviewed changes

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

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.

Comment threadlib/internal/child_process.js Outdated

@bnoordhuisbnoordhuisMay 2, 2018

Copy link
Copy Markdown
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.

Comment threadtest/parallel/test-child-process-disconnect.js Outdated

@bnoordhuisbnoordhuisMay 2, 2018

Copy link
Copy Markdown
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

fhinkel commented Jun 5, 2018

Copy link
Copy Markdown
Member

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

@ryzokuken

ryzokuken commented Jun 7, 2018

Copy link
Copy Markdown
Contributor

ping @laino

@maclover7

maclover7 commented Aug 11, 2018

Copy link
Copy Markdown
Contributor

ping @laino

@maclover7maclover7 added the wip Issues and PRs that are still a work in progress. label Aug 11, 2018
@laino
lainoforce-pushed the child-process-close branch from fff9957 to 74ceb62CompareSeptember 8, 2018 15:21
@laino

laino commented Sep 8, 2018

Copy link
Copy Markdown
Author

Implemented the requested changes and rebased onto the current master.

@lundibundi

lundibundi commented Oct 1, 2018

Copy link
Copy Markdown
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
Comment threadtest/parallel/test-child-process-close-handle-queue.js Outdated

@lundibundilundibundiOct 1, 2018

Copy link
Copy Markdown
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.

Comment threadtest/parallel/test-child-process-close-handle-queue.js Outdated

@lundibundilundibundiOct 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

assert.ifError( ?

@lundibundi

lundibundi commented Oct 8, 2018

Copy link
Copy Markdown
Member

ping @laino

@jasnelljasnell 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
Trottforce-pushed the child-process-close branch from 74ceb62 to 40a840dCompareNovember 10, 2018 23:59
@Trott

Trott commented Nov 11, 2018

Copy link
Copy Markdown
Member

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

@fhinkel

fhinkel commented Oct 26, 2019

Copy link
Copy Markdown
Member

Closing this due to inactivity. Please reopen if needed.

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

Reviewers

@TrottTrottTrott left review comments
@tniessentniessentniessen left review comments
@BridgeARBridgeARBridgeAR left review comments
@bnoordhuisbnoordhuisbnoordhuis approved these changes
@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxAwaiting requested review from addaleax
+1 more reviewer
@lundibundilundibundilundibundi left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

child_processIssues and PRs related to the child_process subsystem.stalledIssues and PRs that are stalled.wipIssues 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

11 participants

@laino@BridgeAR@fhinkel@ryzokuken@maclover7@lundibundi@Trott@bnoordhuis@jasnell@tniessen@nodejs-github-bot