Skip to content
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.

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

internal/child_process: call postSend on error #4752

Conversation

Projects
None yet
7 participants
@indutny
Copy link
Member

commented Jan 19, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2016

@sam-github

This comment has been minimized.

Copy link
Member

commented Jan 21, 2016

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

@indutny

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2016

@sam-github no worries

@bnoordhuis you are my last hope

@trevnorris

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2016

@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 indutny force-pushed the indutny:fix/handle-cleanup-in-child-process-send branch Jan 21, 2016

@indutny

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2016

@trevnorris how about this?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Jan 21, 2016

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
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 indutny force-pushed the indutny:fix/handle-cleanup-in-child-process-send branch to 994071b Jan 22, 2016

@indutny

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2016

@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

This comment has been minimized.

Copy link
Member

commented Jan 22, 2016

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

@jasnell

This comment has been minimized.

Copy link
Member

commented Jan 22, 2016

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

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Jan 22, 2016

Commit log LGTM.

@indutny

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2016

@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

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2016

Landed in 6712a1f, thank you!

@indutny 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
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 added a commit that referenced this pull request Jan 25, 2016

internal/child_process: call postSend on error
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 added a commit that referenced this pull request Feb 17, 2016

internal/child_process: call postSend on error
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 added a commit that referenced this pull request Feb 18, 2016

internal/child_process: call postSend on error
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 MylesBorins referenced this pull request Feb 18, 2016

Merged

V4.4.0 proposal #5301

MylesBorins added a commit that referenced this pull request Mar 2, 2016

internal/child_process: call postSend on error
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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.