Skip to content

cluster: reset handle index on close#6981

Merged
santigimeno merged 1 commit intonodejs:masterfrom
santigimeno:bug_6693
May 27, 2016
Merged

cluster: reset handle index on close#6981
santigimeno merged 1 commit intonodejs:masterfrom
santigimeno:bug_6693

Conversation

@santigimeno
Copy link
Member

@santigimeno santigimeno commented May 25, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)
Description of change

It allows reopening a server after it has been closed.

Fixes: #6693.

/cc @cjihrig @bnoordhuis

@nodejs-github-bot nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label May 25, 2016
@santigimeno
Copy link
Member Author

santigimeno commented May 26, 2016

CI: https://ci.nodejs.org/job/node-test-commit/3504/

bnoordhuis
bnoordhuis reviewed May 26, 2016
View reviewed changes
lib/cluster.js Outdated
Copy link
Member

@bnoordhuis bnoordhuis May 26, 2016

Choose a reason for hiding this comment

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

This looks too hacky. I'd rename the const key = ... at the top of cluster._getServer() to const indexesKey and use that here.

Also, this would be a place where the delete operator is warranted, otherwise indexes can grow unchecked.

Copy link
Member Author

@santigimeno santigimeno May 26, 2016

Choose a reason for hiding this comment

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

This looks too hacky. I'd rename the const key = ... at the top of cluster._getServer() to const indexesKey and use that here.

Maybe I'm missing something, but how would I do that? They're in different contexts. I can pass it to the shared and rr functions as an argument though.

Also, this would be a place where the delete operator is warranted, otherwise indexes can grow unchecked.

You're right. Will fix.

Thanks!

@santigimeno
Copy link
Member Author

santigimeno commented May 26, 2016

PR updated with @bnoordhuis suggestions. Thanks!

@santigimeno
Copy link
Member Author

santigimeno commented May 26, 2016

CI: https://ci.nodejs.org/job/node-test-commit/3513/

bnoordhuis
bnoordhuis reviewed May 26, 2016
View reviewed changes
lib/cluster.js Outdated
Copy link
Member

@bnoordhuis bnoordhuis May 26, 2016

Choose a reason for hiding this comment

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

I'd rename key to indexesKey for better greppability.

@santigimeno
Copy link
Member Author

santigimeno commented May 26, 2016

Updated. Thanks!

@santigimeno
Copy link
Member Author

santigimeno commented May 26, 2016

Ci: https://ci.nodejs.org/job/node-test-pull-request/2807/

@bnoordhuis
Copy link
Member

bnoordhuis commented May 26, 2016 •
edited
Loading

LGTM. armv7-ubuntu1404 has a lot of failures but they are probably caused by something else. I suspect a debugger test.

cjihrig
cjihrig reviewed May 26, 2016
View reviewed changes
test/parallel/test-cluster-server-restart-rr.js Outdated
Copy link
Contributor

@cjihrig cjihrig May 26, 2016

Choose a reason for hiding this comment

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

Could this be a common.mustCall()? It might be good to test worker1's exit code too.

@cjihrig
Copy link
Contributor

cjihrig commented May 26, 2016

LGTM with one comment.

@santigimeno
Copy link
Member Author

santigimeno commented May 26, 2016

PR update per @cjihrig comments. Thanks

@santigimeno
Copy link
Member Author

santigimeno commented May 26, 2016

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

@ronkorving
Copy link
Contributor

ronkorving commented May 27, 2016

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented May 27, 2016

LGTM

@santigimeno
Copy link
Member Author

santigimeno commented May 27, 2016

One last CI: https://ci.nodejs.org/job/node-test-pull-request/2824/

@santigimeno
Copy link
Member Author

santigimeno commented May 27, 2016

CI looks good except for the current flakes. Landing.

@santigimeno
cluster: reset handle index on close
0c29436 
It allows reopening a server after it has been closed.

Fixes: nodejs#6693
PR-URL: nodejs#6981
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@santigimeno santigimeno merged commit 0c29436 into nodejs:master May 27, 2016
@santigimeno
Copy link
Member Author

santigimeno commented May 27, 2016

Landed in 0c29436. Thanks!

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
@santigimeno @Fishrock123
cluster: reset handle index on close
213a03b 
It allows reopening a server after it has been closed.

Fixes: nodejs#6693
PR-URL: nodejs#6981
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
@santigimeno @rvagg
cluster: reset handle index on close
0bd8f4c 
It allows reopening a server after it has been closed.

Fixes: #6693
PR-URL: #6981
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 11, 2016
Closed
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 11, 2016

@santigimeno / @bnoordhuis lts?

@santigimeno
Copy link
Member Author

santigimeno commented Jul 12, 2016

@thealphanerd, yes.

MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@santigimeno
cluster: reset handle index on close
27222f4 
It allows reopening a server after it has been closed.

Fixes: #6693
PR-URL: #6981
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@santigimeno
cluster: reset handle index on close
31756f4 
It allows reopening a server after it has been closed.

Fixes: #6693
PR-URL: #6981
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@santigimeno
cluster: reset handle index on close
27b5512 
It allows reopening a server after it has been closed.

Fixes: #6693
PR-URL: #6981
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@santigimeno
cluster: reset handle index on close
20d3378 
It allows reopening a server after it has been closed.

Fixes: #6693
PR-URL: #6981
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.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

Reviewers

No reviews

Assignees

No one assigned

Labels

cluster Issues and PRs related to the cluster subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cluster workers not sharing ports after reopening a listener

7 participants

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