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

internal/child_process: call postSend on error by indutny · Pull Request #4752 · nodejs/node · GitHub

/ node Public

internal/child_process: call postSend on error - #4752

Closed
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:fix/handle-cleanup-in-child-process-send
ConversationCommits (1)ChecksFiles changed
Closed

internal/child_process: call postSend on error#4752
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:fix/handle-cleanup-in-child-process-send

Conversation

indutny commented Jan 19, 2016

Copy link
Copy Markdown
Member

Call obj.postSend in error case of process.send(). The
net.Socket's handle should not be leaked.

Noticed this while looking through the code. I'm not sure if any reliable test case could be written for it, but the problem seems to be kind of obvious to me.

R = @bnoordhuis or @sam-github

cc @nodejs/collaborators

indutny commented Jan 19, 2016

Copy link
Copy Markdown
Member Author

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

mscdex added the child_process Issues and PRs related to the child_process subsystem. label Jan 19, 2016

sam-github commented Jan 21, 2016

Copy link
Copy Markdown
Contributor

@indutny sorry, I'm not familiar enough with the code to comment

indutny commented Jan 21, 2016

Copy link
Copy Markdown
Member Author

@sam-github no worries

@bnoordhuis you are my last hope

trevnorris commented Jan 21, 2016

Copy link
Copy Markdown
Contributor

@indutny With the short commit message and lack of test I'm having a hard time seeing this in context. Will be happy to review if at least one of those two can be improved/added.

indutny force-pushed the fix/handle-cleanup-in-child-process-send branch from 998145a to 1feb860 Compare January 21, 2016 07:55

indutny commented Jan 21, 2016

Copy link
Copy Markdown
Member Author

@trevnorris how about this?

bnoordhuis commented Jan 21, 2016

Copy link
Copy Markdown
Member

LGTM but "certain callbacks invoked" sounds a bit vague; I'd write "two callbacks are invoked, send and postSend."

(Aside, it's "These callbacks", not "This callbacks.")

internal/child_process: call postSend on error
994071b 
Call `obj.postSend` in error case of `process.send()`. The
`net.Socket`'s handle should not be leaked.

Note that there are two callbacks invoked on handles
when they are sent to the child process over IPC pipes.
These callbacks are specified by `handleConversion` object, and
during send two of them are invoked:

  * `send`
  * `postSend`

Now for `net.Socket` in particular, `postSend` performs clean up by
closing the actual uv handle. However this clean up will not happen in
one of the branches. This pull request aims to fix this.
indutny force-pushed the fix/handle-cleanup-in-child-process-send branch from 1feb860 to 994071b Compare January 22, 2016 01:33

indutny commented Jan 22, 2016

Copy link
Copy Markdown
Member Author

@bnoordhuis thank you! Does this one look better? I didn't want to repeat postSend/send, so I tried to do it a bit differently

jasnell commented Jan 22, 2016

Copy link
Copy Markdown
Member

If I'm understanding this one correctly, then LGTM but would prefer additional sign off

jasnell commented Jan 22, 2016

Copy link
Copy Markdown
Member

marking as lts-watch defensively... but not sure it's appropriate to land there yet. Need more input

bnoordhuis commented Jan 22, 2016

Copy link
Copy Markdown
Member

Commit log LGTM.

indutny commented Jan 22, 2016

Copy link
Copy Markdown
Member Author

@jasnell hm... it should be probably a good idea, though, I'm not sure if it matters that much for LTS. Never seen a bug that caused this, this is just a sanity check commit.

indutny commented Jan 22, 2016

Copy link
Copy Markdown
Member Author

Landed in 6712a1f, thank you!

indutny closed this Jan 22, 2016
indutny added a commit that referenced this pull request Jan 22, 2016
internal/child_process: call postSend on error
6712a1f 
Call `obj.postSend` in error case of `process.send()`. The
`net.Socket`'s handle should not be leaked.

Note that there are two callbacks invoked on handles
when they are sent to the child process over IPC pipes.
These callbacks are specified by `handleConversion` object, and
during send two of them are invoked:

  * `send`
  * `postSend`

Now for `net.Socket` in particular, `postSend` performs clean up by
closing the actual uv handle. However this clean up will not happen in
one of the branches. This pull request aims to fix this.

PR-URL: #4752
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Jan 25, 2016
internal/child_process: call postSend on error
b2c8b7f 
Call `obj.postSend` in error case of `process.send()`. The
`net.Socket`'s handle should not be leaked.

Note that there are two callbacks invoked on handles
when they are sent to the child process over IPC pipes.
These callbacks are specified by `handleConversion` object, and
during send two of them are invoked:

  * `send`
  * `postSend`

Now for `net.Socket` in particular, `postSend` performs clean up by
closing the actual uv handle. However this clean up will not happen in
one of the branches. This pull request aims to fix this.

PR-URL: #4752
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
internal/child_process: call postSend on error
7cbb484 
Call `obj.postSend` in error case of `process.send()`. The
`net.Socket`'s handle should not be leaked.

Note that there are two callbacks invoked on handles
when they are sent to the child process over IPC pipes.
These callbacks are specified by `handleConversion` object, and
during send two of them are invoked:

  * `send`
  * `postSend`

Now for `net.Socket` in particular, `postSend` performs clean up by
closing the actual uv handle. However this clean up will not happen in
one of the branches. This pull request aims to fix this.

PR-URL: #4752
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
internal/child_process: call postSend on error
360e04f 
Call `obj.postSend` in error case of `process.send()`. The
`net.Socket`'s handle should not be leaked.

Note that there are two callbacks invoked on handles
when they are sent to the child process over IPC pipes.
These callbacks are specified by `handleConversion` object, and
during send two of them are invoked:

  * `send`
  * `postSend`

Now for `net.Socket` in particular, `postSend` performs clean up by
closing the actual uv handle. However this clean up will not happen in
one of the branches. This pull request aims to fix this.

PR-URL: #4752
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
internal/child_process: call postSend on error
d6894f8 
Call `obj.postSend` in error case of `process.send()`. The
`net.Socket`'s handle should not be leaked.

Note that there are two callbacks invoked on handles
when they are sent to the child process over IPC pipes.
These callbacks are specified by `handleConversion` object, and
during send two of them are invoked:

  * `send`
  * `postSend`

Now for `net.Socket` in particular, `postSend` performs clean up by
closing the actual uv handle. However this clean up will not happen in
one of the branches. This pull request aims to fix this.

PR-URL: #4752
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
internal/child_process: call postSend on error
bc1e346 
Call `obj.postSend` in error case of `process.send()`. The
`net.Socket`'s handle should not be leaked.

Note that there are two callbacks invoked on handles
when they are sent to the child process over IPC pipes.
These callbacks are specified by `handleConversion` object, and
during send two of them are invoked:

  * `send`
  * `postSend`

Now for `net.Socket` in particular, `postSend` performs clean up by
closing the actual uv handle. However this clean up will not happen in
one of the branches. This pull request aims to fix this.

PR-URL: nodejs#4752
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
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

No reviews

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL