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

cluster: remove handles when disconnecting worker #3677

Merged
merged 1 commit into from
Nov 6, 2015

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis added the cluster Issues and PRs related to the cluster subsystem. label Nov 5, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Nov 5, 2015

Looks like the CI had some issues with the test.

@bnoordhuis
Copy link
Member Author

Fix-ups applied. New run: https://ci.nodejs.org/job/node-test-pull-request/675/

@cjihrig
Copy link
Contributor

cjihrig commented Nov 5, 2015

LGTM, and I think the CI is ok. Looks like some Windows slaves went offline.

'use strict';

// Used in tests.
exports.handles = {};
Copy link
Member

Choose a reason for hiding this comment

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

Time for Map?

Copy link
Contributor

Choose a reason for hiding this comment

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

@indutny
Copy link
Member

indutny commented Nov 5, 2015

Commit log is pretty shallow, may I ask you to add more information to it?

@indutny
Copy link
Member

indutny commented Nov 5, 2015

Otherwise LGTM

@jasnell
Copy link
Member

jasnell commented Nov 5, 2015

LGTM

Due to the race window between the master's "disconnect" message and the
worker's "handle received" message, connections sometimes got stuck in
the pending handles queue when calling `worker.disconnect()` in the
master process.

The observable effect from the client's perspective was a TCP or HTTP
connection that simply stalled.  This commit fixes that by closing open
handles in the master when the "disconnect" message is sent.

Fixes: nodejs#3551
PR-URL: nodejs#3677
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis
Copy link
Member Author

Tagged lts-watch-v4.x but can I suggest letting this brew in master / v5 for 10 days or so?

@jasnell
Copy link
Member

jasnell commented Nov 6, 2015

+1. Not planning another LTS update round for at least another week
On Nov 6, 2015 2:19 PM, "Ben Noordhuis" notifications@github.com wrote:

Tagged lts-watch-v4.x but can I suggest letting this brew in master / v5
for 10 days or so?


Reply to this email directly or view it on GitHub
#3677 (comment).

bnoordhuis added a commit that referenced this pull request Nov 7, 2015
Due to the race window between the master's "disconnect" message and the
worker's "handle received" message, connections sometimes got stuck in
the pending handles queue when calling `worker.disconnect()` in the
master process.

The observable effect from the client's perspective was a TCP or HTTP
connection that simply stalled.  This commit fixes that by closing open
handles in the master when the "disconnect" message is sent.

Fixes: #3551
PR-URL: #3677
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@narqo
Copy link

narqo commented Nov 7, 2015

Any chance this to be ported to v0.12?

@jasnell
Copy link
Member

jasnell commented Nov 7, 2015

Possibly. I put it on the watch list so it can be evaluated

This was referenced Nov 10, 2015
@MylesBorins
Copy link
Contributor

@bnoordhuis do you think it is ready to land in v4.x?

@bnoordhuis
Copy link
Member Author

@thealphanerd Maybe wait a little longer, it hasn't made its way into a release yet. It's scheduled for v5.1.0, see #3736 (comment).

@jasnell
Copy link
Member

jasnell commented Nov 17, 2015

Yeah, likely best to hold off. Just in case.

@bnoordhuis ... keep in mind that @thealphanerd was only talking about landing in v4.x-staging (right @thealphanerd ?). That would mean that it would still be a couple weeks before it actually goes out in a v4.x release.

@MylesBorins
Copy link
Contributor

indeed

@bnoordhuis
Copy link
Member Author

Oh, in that case go for it.

bnoordhuis added a commit that referenced this pull request Nov 30, 2015
Due to the race window between the master's "disconnect" message and the
worker's "handle received" message, connections sometimes got stuck in
the pending handles queue when calling `worker.disconnect()` in the
master process.

The observable effect from the client's perspective was a TCP or HTTP
connection that simply stalled.  This commit fixes that by closing open
handles in the master when the "disconnect" message is sent.

Fixes: #3551
PR-URL: #3677
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bnoordhuis added a commit that referenced this pull request Dec 4, 2015
Due to the race window between the master's "disconnect" message and the
worker's "handle received" message, connections sometimes got stuck in
the pending handles queue when calling `worker.disconnect()` in the
master process.

The observable effect from the client's perspective was a TCP or HTTP
connection that simply stalled.  This commit fixes that by closing open
handles in the master when the "disconnect" message is sent.

Fixes: #3551
PR-URL: #3677
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request Dec 17, 2015
bnoordhuis added a commit that referenced this pull request Dec 17, 2015
Due to the race window between the master's "disconnect" message and the
worker's "handle received" message, connections sometimes got stuck in
the pending handles queue when calling `worker.disconnect()` in the
master process.

The observable effect from the client's perspective was a TCP or HTTP
connection that simply stalled.  This commit fixes that by closing open
handles in the master when the "disconnect" message is sent.

Fixes: #3551
PR-URL: #3677
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bnoordhuis added a commit that referenced this pull request Dec 23, 2015
Due to the race window between the master's "disconnect" message and the
worker's "handle received" message, connections sometimes got stuck in
the pending handles queue when calling `worker.disconnect()` in the
master process.

The observable effect from the client's perspective was a TCP or HTTP
connection that simply stalled.  This commit fixes that by closing open
handles in the master when the "disconnect" message is sent.

Fixes: #3551
PR-URL: #3677
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
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
Labels
cluster Issues and PRs related to the cluster subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants