FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

child_process: fix channel disconnect logic by laino · Pull Request #19566 · nodejs/node · GitHub

/ node Public

child_process: fix channel disconnect logic - #19566

Closed
laino wants to merge 1 commit into
nodejs:masterfrom
laino:child-process-close
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

laino 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-bot added the child_process Issues and PRs related to the child_process subsystem. label Mar 23, 2018
laino force-pushed the child-process-close branch from 0943fd3 to 49c68b9 Compare March 23, 2018 20:03
jasnell requested review from addaleax and bnoordhuis March 23, 2018 20:06

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 force-pushed the child-process-close branch 3 times, most recently from c596738 to fff9957 Compare March 24, 2018 04:39
laino changed the title child_process: fix channel disconnect logic & cleanup pending handles child_process: fix channel disconnect logic Mar 24, 2018

BridgeAR commented Apr 9, 2018

Copy link
Copy Markdown
Member

@nodejs/child_process @addaleax @bnoordhuis PTAL

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 reviewed May 2, 2018
View reviewed changes

tniessen 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

bnoordhuis 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Left some comments but apart from that LGTM.

Comment thread lib/internal/child_process.js Outdated

bnoordhuis May 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 thread test/parallel/test-child-process-disconnect.js Outdated

bnoordhuis May 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

fhinkel commented Jun 5, 2018

Copy link
Copy Markdown
Contributor

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

ryzokuken commented Jun 7, 2018

Copy link
Copy Markdown
Contributor

ping @laino

maclover7 commented Aug 11, 2018

Copy link
Copy Markdown
Contributor

ping @laino

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

laino commented Sep 8, 2018

Copy link
Copy Markdown
Author

Implemented the requested changes and rebased onto the current master.

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 reviewed Oct 1, 2018
View reviewed changes
Comment thread test/parallel/test-child-process-close-handle-queue.js Outdated

lundibundi Oct 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

lundibundi Oct 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

assert.ifError( ?

lundibundi commented Oct 8, 2018

Copy link
Copy Markdown
Member

ping @laino

jasnell added the stalled Issues and PRs that are stalled. label Oct 16, 2018
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 force-pushed the child-process-close branch from 74ceb62 to 40a840d Compare November 10, 2018 23:59

Trott commented Nov 11, 2018

Copy link
Copy Markdown
Member

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

fhinkel commented Oct 26, 2019

Copy link
Copy Markdown
Contributor

Closing this due to inactivity. Please reopen if needed.

fhinkel closed this Oct 26, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott left review comments
tniessen tniessen left review comments
BridgeAR BridgeAR left review comments
bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
addaleax Awaiting requested review from addaleax
+1 more reviewer
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
Back | FazBrowse Home | New Git URL