Skip to content

Conversation

@santigimeno
Copy link
Member

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

cluster

Description of change

Avoid sending messages if the IPC channel is already disconnected.
It avoids undesired errors when calling process.disconnect when there
are still pending IPC messages.

/cc @cjihrig@bnoordhuis

@nodejs-github-botnodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Jun 3, 2016
@santigimeno
Copy link
MemberAuthor

@jasnell
Copy link
Member

LGTM

lib/cluster.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

The diff would be a little less noisy if you did:

if(!proc.connected)returnfalse;

@cjihrig
Copy link
Contributor

LGTM, I just hope there aren't scenarios I'm not thinking of where this could swallow valid errors.

@santigimenosantigimenoforce-pushed the cluster_process_disconnect branch from f54ce27 to 6e838e9CompareJune 5, 2016 09:23
@santigimeno
Copy link
MemberAuthor

Updated. Thanks!

@santigimeno
Copy link
MemberAuthor

@santigimenosantigimenoforce-pushed the cluster_process_disconnect branch 2 times, most recently from 381d514 to de6ff56CompareJune 8, 2016 12:48
@santigimeno
Copy link
MemberAuthor

Rebased to current master. One last CI run: https://ci.nodejs.org/job/node-test-pull-request/2957/

@santigimeno
Copy link
MemberAuthor

All is green. Landing

Avoid sending messages if the IPC channel is already disconnected. It avoids undesired errors when calling `process.disconnect` when there are still pending IPC messages. PR-URL: nodejs#7132 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@santigimenosantigimenoforce-pushed the cluster_process_disconnect branch from de6ff56 to 8c53d2fCompareJune 8, 2016 13:51
@santigimenosantigimeno merged commit 8c53d2f into nodejs:masterJun 8, 2016
@santigimeno
Copy link
MemberAuthor

Landed in 8c53d2f. Thanks!

evanlucas pushed a commit that referenced this pull request Jun 16, 2016
Avoid sending messages if the IPC channel is already disconnected. It avoids undesired errors when calling `process.disconnect` when there are still pending IPC messages. PR-URL: #7132 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@evanlucasevanlucas mentioned this pull request Jun 16, 2016
@MylesBorins
Copy link
Contributor

@santigimeno lts?

@santigimeno
Copy link
MemberAuthor

@thealphanerd Yes, I would think so.

MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Avoid sending messages if the IPC channel is already disconnected. It avoids undesired errors when calling `process.disconnect` when there are still pending IPC messages. PR-URL: #7132 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Avoid sending messages if the IPC channel is already disconnected. It avoids undesired errors when calling `process.disconnect` when there are still pending IPC messages. PR-URL: #7132 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Avoid sending messages if the IPC channel is already disconnected. It avoids undesired errors when calling `process.disconnect` when there are still pending IPC messages. PR-URL: #7132 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Avoid sending messages if the IPC channel is already disconnected. It avoids undesired errors when calling `process.disconnect` when there are still pending IPC messages. PR-URL: #7132 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clusterIssues and PRs related to the cluster subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@santigimeno@jasnell@cjihrig@MylesBorins@nodejs-github-bot