net: allow cluster workers to listen on exclusive ports #8238
Conversation
Currently, all listen() calls go through the cluster master. This commit allows workers to listen on ports exclusively. Closes #3856
|
Yes please. Hoping this can land in 0.12. Dx |
| function listen(self, address, port, addressType, backlog, fd) { | ||
| function listen(self, address, port, addressType, backlog, fd, shared) { | ||
| // listening still goes through master by default | ||
| shared = shared === undefined ? true : !!shared; |
|
LGTM, please fix minor nits. Thank you! |
|
@indutny I've addressed your comments. Thanks. |
| server.listen({ | ||
| host: 'localhost', | ||
| port: 80, | ||
| shared: true |
|
Could you apply also to dgram? We can't actually use dgram on windows right now in cluster workers, but if we could specify shared, we could. |
|
Also, this doesn't fix for local domain sockets, either, does it? You can't do |
| function listen(self, address, port, addressType, backlog, fd) { | ||
| function listen(self, address, port, addressType, backlog, fd, shared) { | ||
| // listening still goes through master by default | ||
| shared = shared !== false; |
There was a problem hiding this comment.
You consider 0, null, and undefined to be true? I think this is weird. I suggest that this should be shared = !!shared
There was a problem hiding this comment.
I originally had:
shared = shared === undefined ? true : !!shared;
I was asked to change it. undefined is a bit of a special case because the shared argument is not always passed, but shared should default to true. I think the simplest solution would be to explicitly pass all arguments, which is achievable since this is an internal API.
EDIT: I responded to this before reading your next comment. It may be simpler to switch to from shared to exclusive.
|
I like this, and to see it consistently applied to all the sockets that cluster shares, not just tcp. |
Currently, all dgram bind() calls go through the cluster master. This commit allows workers to bind to a port exclusively.
|
@sam-github updated with support for There is one case that is still not covered - listening on an existing handle/fd. Adding support wouldn't be too hard. The biggest problem is what to call the property. I can't use |
| @@ -152,6 +152,7 @@ function replaceHandle(self, newHandle) { | |||
|
|
|||
| Socket.prototype.bind = function(/*port, address, callback*/) { | |||
| var self = this; | |||
| var firstArg = arguments[0]; | |||
There was a problem hiding this comment.
If this is immediately being set, why not just place it in the argument declaration?
|
@trevnorris I have addressed your comments. |
| * `callback` {Function} - Optional. | ||
|
|
||
| The `port`, `host`, and `backlog` properties of `options`, as well as the | ||
| optional callback function, behave as they do on a call to |
|
@trevnorris - updated |
|
Thanks. Squashed, slightly re-worded and landed in 029cfc1. |
Currently, all
listen()calls go through the cluster master. This commit allows workers to listen on ports exclusively. Closes #3856