-
Notifications
You must be signed in to change notification settings - Fork 30k
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, net: backlog need pass to listen #4056
Conversation
would you add some test? |
Can you update the documentation as well? If I read your patch right, the first backlog value wins when workers request different values. That should be documented. |
else | ||
this.server.listen(address); // UNIX socket path. | ||
this.server.listen(address, backlog); // UNIX socket path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all of the variations of listen()
accept a backlog
parameter? It doesn't look like it's documented that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks correct to me. Here is the relevant code in lib/net.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, the code checks out. The documentation is incomplete :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to server.listen's implementation, if second or third argument
is a Number, treat it as backlog size.
But server.listen(options[, callback]) already have a backlog in options
and has higher priority, it is better to avoid document it has another
standalone backlog parameter to avoid more confuse.
@tangxinfa Left a comment. Note that GitHub doesn't send notifications when you add or change commits so please post a comment when you're done. |
@tangxinfa .. haven't reviewed this but the PR would need to be cleaned up before it can land. It looks like you may have done a git merge instead of a rebase? |
I cleaned up my repo, please checkout, and i also create another branch(https://github.com/tangxinfa/node/tree/cluster-support-backlog) combined three commits to one(tangxinfa@a1da62a) if you like:) |
@tangxinfa .. thank you. Sorry I hadn't seen the additional commits, Github unfortunately does not notify when commits are added so at times we end up missing things. This LGTM if @bnoordhuis and @cjihrig are happy with it |
@JungMinu backlog is a hint to the implementationman listen page says:
node doc says:
There is no standard api to retrieve the backlog option from a listened socket, but you can watch it from tools, test with the following example program(http-hello-cluster.js):
"Send-Q" field from "ss -tln" command output
"listen" system call from "strace" command output
backlog is not store in the handleSo we can't do listen on slave with a backlog, retrieve the backlog from the handle after listened, and compare it to verify it works. |
The backlog parameter is supported by all variations of net.Server.listen(), but wasn't consistently documented. This commit brings the documentation into a more consistent state. Refs: #4056 PR-URL: #4025 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
The backlog parameter is supported by all variations of net.Server.listen(), but wasn't consistently documented. This commit brings the documentation into a more consistent state. Refs: #4056 PR-URL: #4025 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
The backlog parameter is supported by all variations of net.Server.listen(), but wasn't consistently documented. This commit brings the documentation into a more consistent state. Refs: #4056 PR-URL: #4025 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
The backlog parameter is supported by all variations of net.Server.listen(), but wasn't consistently documented. This commit brings the documentation into a more consistent state. Refs: #4056 PR-URL: #4025 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
The backlog parameter is supported by all variations of net.Server.listen(), but wasn't consistently documented. This commit brings the documentation into a more consistent state. Refs: nodejs#4056 PR-URL: nodejs#4025 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
7da4fd4
to
c7066fb
Compare
@tangxinfa are you still interested in this? If so, it needs a rebase. |
@cjihrig Please have a look, thanks:) |
@tangxinfa you should be able to rebase on top of the same branch. You'll just have to resolve some merge conflicts. Alternatively, you could open a separate PR with your cluster-support-backlog branch. |
I'm seeing the same issue, any update on this? |
Currently, the master process would ignore `backlog` from worker processes and use the default value instead. This commit will respect the first `backlog` passed to the master process for a specific handle. Refs: nodejs#4056
Currently, the master process would ignore `backlog` from worker processes and use the default value instead. This commit will respect the first `backlog` passed to the master process for a specific handle. Refs: nodejs#4056
net.Server.listen's backlog option not take effect when use cluster, the backlog always 511(default value).
There is a two example, i set backlog to 64:
Without cluster:
ss -ltn | grep 3000 output:
With cluster
ss -ltn | grep 3000 output:
The backlog take effect is 511 not 64, this means node cluster module NOT take care of the backlog option when we call listen: server.listen(port[, hostname][, backlog][, callback])