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.

Sign up for GitHub

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

Jump to bottom

cluster,dgram: pass UDP bind flags from cluster worker #2611

Closed
wants to merge 1 commit into from
Closed

cluster,dgram: pass UDP bind flags from cluster worker #2611

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Aug 29, 2015

Previously, reuseAddr: true was being ignored when set inside a cluster worker. This commit passes the bind flags to the master so that the socket is bound appropriately.

Fixes: #2604

@mscdex mscdex added cluster Issues and PRs related to the cluster subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. labels Aug 29, 2015
cjihrig
cjihrig reviewed Aug 29, 2015
View reviewed changes
lib/dgram.js
// Opening an existing fd is not supported for UDP handles.
assert(typeof fd !== 'number' || fd < 0);

var handle = newHandle(addressType);

if (port || address) {
var err = handle.bind(address, port || 0, 0);
var err = handle.bind(address, port || 0, flags);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you replace the vars with let and const where appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the whole file or just this one instance? Maybe that's best handled in a separate PR to avoid extra noise?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just the things that you're actually touching in this PR so there is no extra noise.

bnoordhuis
bnoordhuis reviewed Aug 29, 2015
View reviewed changes
lib/dgram.js
// Opening an existing fd is not supported for UDP handles.
assert(typeof fd !== 'number' || fd < 0);

var handle = newHandle(addressType);

if (port || address) {
var err = handle.bind(address, port || 0, 0);
let err = handle.bind(address, port || 0, flags);
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit inconsistent to change var to let here, then use var in newly added code below.

EDIT: I see @cjihrig asked you to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I meant in the context of the whole PR, not just this line.

@mscdex
Copy link
Contributor Author

mscdex commented Aug 30, 2015

Ok, I've made the suggested improvements. However I still don't know why Windows fails with:

Error: write ENOTSUP
    at exports._errnoException (util.js:837:11)
    at ChildProcess.target._send (internal/child_process.js:552:28)
    at ChildProcess.target.send (internal/child_process.js:485:12)
    at sendHelper (cluster.js:688:8)
    at send (cluster.js:497:5)
    at cluster.js:473:7
    at SharedHandle.add (cluster.js:82:3)
    at queryServer (cluster.js:465:12)
    at Worker.onmessage (cluster.js:422:7)
    at ChildProcess.<anonymous> (cluster.js:702:8)

@mscdex
cluster,dgram: pass UDP bind flags from cluster worker
8af5f07 
Previously, `reuseAddr: true` was being ignored when set inside a
cluster worker. This commit passes the bind flags to the master so
that the socket is bound appropriately.
@mscdex
Copy link
Contributor Author

mscdex commented Aug 30, 2015

Ok, so it's the passing of UDP server handles that isn't supported on Windows, not reuseAddr: true specifically. I've updated the test output message for Windows.

@mscdex
Copy link
Contributor Author

mscdex commented Sep 8, 2015

Already fixed by c7be08c.

@mscdex mscdex closed this Sep 8, 2015
@mscdex mscdex deleted the fix-cluster-reuseaddr branch September 8, 2015 07:15
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. dgram Issues and PRs related to the dgram subsystem / UDP.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mscdex @bnoordhuis @cjihrig