cluster: reset handle index on close #6981
Conversation
nodejs-github-bot
added
the
cluster
| @@ -636,6 +638,8 @@ function workerInit() { | |||
| if (key === undefined) return; | |||
| send({ act: 'close', key: key }); | |||
| delete handles[key]; | |||
| const indexesKey = key.substring(0, key.lastIndexOf(':')); | |||
| indexes[indexesKey] = undefined; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
|
PR updated with @bnoordhuis suggestions. Thanks! |
| else | ||
| rr(reply, cb); // Round-robin. | ||
| rr(reply, key, cb); // Round-robin. |
There was a problem hiding this comment.
I'd rename key to indexesKey for better greppability.
|
Updated. Thanks! |
|
LGTM. armv7-ubuntu1404 has a lot of failures but they are probably caused by something else. I suspect a debugger test. |
| const worker1 = cluster.fork(); | ||
| worker1.on('listening', common.mustCall(() => { | ||
| const worker2 = cluster.fork(); | ||
| worker2.on('exit', (code) => { |
There was a problem hiding this comment.
Could this be a common.mustCall()? It might be good to test worker1's exit code too.
|
LGTM with one comment. |
|
PR update per @cjihrig comments. Thanks |
|
LGTM |
1 similar comment
|
LGTM |
|
CI looks good except for the current flakes. Landing. |
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>
|
Landed in 0c29436. Thanks! |
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 / @bnoordhuis lts? |
|
@thealphanerd, yes. |
Checklist
Affected core subsystem(s)
Description of change
It allows reopening a server after it has been closed.
Fixes: #6693.
/cc @cjihrig @bnoordhuis