Don't share dgram sockets that are implicitly bound #8643

sam-github · 2014-10-29T04:56:58Z

Server sockets should be shared by default, and client sockets should be
exclusive by default. For net/TCP, this is how it is, for dgram/UDP, its
a little less clear what a client socket is, but a socket that is
auto-bound during a dgram.send() is not usefully shared among cluster
workers, any more than an outgoing TCP connection would be usefully
shared.

See #8027 (comment)

/to @asilvas @bnoordhuis @rmg

Test triggers a pre-existing bug, so is based on #8642.

asilvas · 2014-10-29T16:03:38Z

+1

rmg · 2014-10-29T17:36:38Z

👍 each commit is a one-liner + tests.

sam-github · 2014-11-14T19:35:21Z

@piscisaureus can you weigh in on this?

Its particularly painful on windows, because cluster doesn't support sharing UPD on windows, so it causes "client" use of UDP to fail on windows with cluster.

greenboxal · 2014-11-15T16:09:55Z

+1
This fixes #8195 although I don't know if this is the right way to fix it. Probably supporting sockets that can be closed and reopened with SharedHandle is a better way.

sam-github · 2014-11-17T20:19:26Z

Probably supporting sockets that can be closed and reopened with SharedHandle is a better way.

@greenboxal

Can you elaborate? I don't really know what that means.

Also, does #8642 alone fix #8195, or do you need the extra functional change in #8643?

greenboxal · 2014-11-17T20:33:59Z

I mean that UDP sockets should be removed when closed, sending the close message to the master or they should just reuse the older socket.

And yes, #8643 is needed in order to fix #8195.

sam-github · 2014-11-26T20:19:32Z

Will fix #5587 if this lands.

misterdjules · 2014-11-26T20:39:24Z

@sam-github Do tests pass on Windows (they weren't passing for #8642)?

sam-github · 2014-12-04T01:52:14Z

@greenboxal I could be a bit confused, because its not clear if you are commenting on #8642 or this PR, #8643.

The only relationship is that the tests for dgram implicit binds being exclusive trigger a completely unrelated bug. I don't really care how that bug gets fixed, but if you have some thoughts, #8642 would be the place to comment.

dgram sockets should NOT be shared at all, in the client use case (no bind).

Currently, if worker A sends request x and worker B sends request y, worker B could get the response to x, and worker A the response to y. Or one could get both responses. This doesn't seem like a feature.

Currently, if you create two dgram2 sockets in one worker, don't bind them, call .send() on both... they both end up with the same underlying shared handle/port! So they will also get intermingled responses. This doesn't seem like a feature either.

Currently, dgram SharedHandles are _not supported at all_ on Windows, so dgram is completely unsupported with cluster on Windows. Attempts to use it, even in cases where its undesireable to share the socket at all, causes the node cluster master to die with ENOTSUP (in a way that is uncatchable...).

This PR fixes the above three problems.

dgram sockets that are explicitly bound are still allocated in the cluster master, even with this PR, causing the cluster master to die on Windows, but fixing that is much more than a one-liner.

piscisaureus · 2014-12-04T23:17:08Z

LGTM

misterdjules · 2014-12-05T22:22:13Z

@sam-github I get this error when running test/simple/test-dgram-exclusive-implicit-bind.js on Windows:

C:\Users\jgilli\dev\node>.\Release\node test\simple\test-dgram-exclusive-implici
t-bind.js
events.js:85
      throw er; // Unhandled 'error' event
            ^
Error: write ENOTSUP
    at exports._errnoException (util.js:746:11)
    at ChildProcess.target._send (child_process.js:484:28)
    at ChildProcess.target.send (child_process.js:416:12)
    at sendHelper (cluster.js:675:8)
    at send (cluster.js:511:5)
    at cluster.js:487:7
    at SharedHandle.add (cluster.js:99:3)
    at queryServer (cluster.js:479:12)
    at Worker.onmessage (cluster.js:437:7)
    at ChildProcess.<anonymous> (cluster.js:691:8)

C:\Users\jgilli\dev\node>

Other than that, it seems like a good change to me, provided that we take care of (potential) issues with #8642 first.

I also think that we should land #8642 before looking at this one again, it will have the added benefit of making the change even smaller and avoid confusion when discussing it.

sam-github · 2014-12-23T22:46:16Z

@misterdjules check this out once #8642 lands, I have verified it on windows and linux, before and after the new behaviour for implictly bound sockets.

misterdjules · 2014-12-24T02:58:44Z

@sam-github Thanks! I don't know if you got that on IRC, but I'm off until January 5th, so I probably won't have time to review it until then.

sam-github · 2015-01-09T01:28:05Z

@misterdjules Happy New Year! Do you have time to take a look at this?

misterdjules · 2015-01-09T21:59:22Z

@sam-github Thanks, happy new year to you too :) I'm taking a look at #8642 first, and then I'll take a look at this one. #8642 looks good so far, and depending on where we are with the merge of v0.10 to v0.12, it'll land before or after it.

How does that sound?

misterdjules · 2015-01-09T22:42:21Z

@sam-github LGTM. This will land after #8642, so we'll need to rebase this branch at some point.

Not landing until we know we're we stand regarding the ongoing merge of v0.10 into v0.12.

Thanks!

misterdjules · 2015-01-09T22:48:26Z

Adding it to the 0.11.15 milestone because it's been reviewed and tested, so it should be quick and easy to land.

sam-github · 2015-01-10T00:54:04Z

I'm happy to rebase, ping me when you want it.

Server sockets should be shared by default, and client sockets should be exclusive by default. For net/TCP, this is how it is, for dgram/UDP, its a little less clear what a client socket is, but a socket that is auto-bound during a dgram.send() is not usefully shared among cluster workers, any more than an outgoing TCP connection would be usefully shared. Since implicit binds become exclusive, implicit/client dgram sockets can now be used with cluster on Windows. Before, neither explicit nor implicitly bound sockets could be used, causing dgram to be completely unsupported with cluster on Windows. After this change, they become half supported. PR-URL: nodejs/node-v0.x-archive#8643 Reviewed-by: Bert Belder <[email protected]>

piscisaureus · 2015-01-10T03:53:51Z

lgtm, landed in io.js: nodejs/node@a32b92d
Another thanks :p

piscisaureus · 2015-01-10T18:32:56Z

The patch was reverted in io.js; nodejs/node#279

piscisaureus · 2015-01-10T19:23:27Z

@sam-github

The patch was reverted because it makes test-cluster-dgram-2 fail. This test hangs because cluster.disconnect() in the master doesn't close the non-shared UDP sockets in the client so they keep the process open.

misterdjules · 2015-01-11T00:04:42Z

@piscisaureus Thanks for the heads-up!

For shared handles that do not get connection close messages (UDP/dgram is the only example of this), cluster must not assume that a port listened on by one worker is listened on by all workers.

Workers that are already disconnected but not yet exited should not be disconnected, trying to do so raises exceptions.

Server sockets should be shared by default, and client sockets should be exclusive by default. For net/TCP, this is how it is, for dgram/UDP, its a little less clear what a client socket is, but a socket that is auto-bound during a dgram.send() is not usefully shared among cluster workers, any more than an outgoing TCP connection would be usefully shared. Since implicit binds become exclusive, implicit/client dgram sockets can now be used with cluster on Windows. Before, neither explicit nor implicitly bound sockets could be used, causing dgram to be completely unsupported with cluster on Windows. After this change, they become half supported.

Server sockets should be shared by default, and client sockets should be exclusive by default. For net/TCP, this is how it is, for dgram/UDP, its a little less clear what a client socket is, but a socket that is auto-bound during a dgram.send() is not usefully shared among cluster workers, any more than an outgoing TCP connection would be usefully shared. Since implicit binds become exclusive, implicit/client dgram sockets can now be used with cluster on Windows. Before, neither explicit nor implicitly bound sockets could be used, causing dgram to be completely unsupported with cluster on Windows. After this change, they become half supported. PR-URL: nodejs/node-v0.x-archive#8643 Reviewed-by: Bert Belder <[email protected]>

sam-github · 2015-01-13T05:22:44Z

Fixed in 0b827a0

sam-github · 2015-01-13T05:33:46Z

cf. nodejs/node#325

Server sockets should be shared by default, and client sockets should be exclusive by default. For net/TCP, this is how it is, for dgram/UDP, its a little less clear what a client socket is, but a socket that is auto-bound during a dgram.send() is not usefully shared among cluster workers, any more than an outgoing TCP connection would be usefully shared. Since implicit binds become exclusive, implicit/client dgram sockets can now be used with cluster on Windows. Before, neither explicit nor implicitly bound sockets could be used, causing dgram to be completely unsupported with cluster on Windows. After this change, they become half supported. PR-URL: nodejs/node-v0.x-archive#8643 Reviewed-by: Bert Belder <[email protected]>

Server sockets should be shared by default, and client sockets should be exclusive by default. For net/TCP, this is how it is, for dgram/UDP, its a little less clear what a client socket is, but a socket that is auto-bound during a dgram.send() is not usefully shared among cluster workers, any more than an outgoing TCP connection would be usefully shared. Since implicit binds become exclusive, implicit/client dgram sockets can now be used with cluster on Windows. Before, neither explicit nor implicitly bound sockets could be used, causing dgram to be completely unsupported with cluster on Windows. After this change, they become half supported. PR: #325 PR: nodejs/node-v0.x-archive#8643 Reviewed-by: Ben Noordhuis <[email protected]> Reviewed-by: Bert Belder <[email protected]>

misterdjules · 2015-02-17T22:19:18Z

@tjfontaine @trevnorris @cjihrig @orangemocha OK to land nodejs/node#325 (the fixed version of this PR) in master?

cjihrig · 2015-02-17T22:21:06Z

+1 from me

trevnorris · 2015-02-18T00:29:27Z

@misterdjules +1

misterdjules · 2015-02-18T00:53:19Z

@cjihrig @trevnorris Thanks! Will land it asap.

Server sockets should be shared by default, and client sockets should be exclusive by default. For net/TCP, this is how it is, for dgram/UDP, its a little less clear what a client socket is, but a socket that is auto-bound during a dgram.send() is not usefully shared among cluster workers, any more than an outgoing TCP connection would be usefully shared. Since implicit binds become exclusive, implicit/client dgram sockets can now be used with cluster on Windows. Before, neither explicit nor implicitly bound sockets could be used, causing dgram to be completely unsupported with cluster on Windows. After this change, they become half supported. PR: nodejs#8643 Reviewed-by: Ben Noordhuis <[email protected]> Reviewed-by: Bert Belder <[email protected]> Reviewed-by: Julien Gilli <[email protected]>

Server sockets should be shared by default, and client sockets should be exclusive by default. For net/TCP, this is how it is, for dgram/UDP, its a little less clear what a client socket is, but a socket that is auto-bound during a dgram.send() is not usefully shared among cluster workers, any more than an outgoing TCP connection would be usefully shared. Since implicit binds become exclusive, implicit/client dgram sockets can now be used with cluster on Windows. Before, neither explicit nor implicitly bound sockets could be used, causing dgram to be completely unsupported with cluster on Windows. After this change, they become half supported. PR: #8643 Reviewed-by: Ben Noordhuis <[email protected]> Reviewed-by: Bert Belder <[email protected]> Reviewed-by: Julien Gilli <[email protected]>

misterdjules · 2015-02-18T02:04:40Z

Landed in e42c4a3. Thanks again @sam-github!

... otherwise it doesn't play nicely with cluster. Similar: nodejs/node-v0.x-archive#8643

Server sockets should be shared by default, and client sockets should be exclusive by default. For net/TCP, this is how it is, for dgram/UDP, its a little less clear what a client socket is, but a socket that is auto-bound during a dgram.send() is not usefully shared among cluster workers, any more than an outgoing TCP connection would be usefully shared. Since implicit binds become exclusive, implicit/client dgram sockets can now be used with cluster on Windows. Before, neither explicit nor implicitly bound sockets could be used, causing dgram to be completely unsupported with cluster on Windows. After this change, they become half supported. PR: nodejs#8643 Reviewed-by: Ben Noordhuis <[email protected]> Reviewed-by: Bert Belder <[email protected]> Reviewed-by: Julien Gilli <[email protected]>

Nodejs-Jenkins added pr v0.12 labels Oct 29, 2014

cjihrig mentioned this pull request Nov 23, 2014

Sending a UDP packet fails within the Cluster environment on Windows #5587

Closed

sam-github mentioned this pull request Dec 4, 2014

Support Windows on node >= 0.11.14 strongloop/strong-agent-statsd#7

Merged

misterdjules added cluster dgram labels Jan 9, 2015

misterdjules added this to the 0.11.15 milestone Jan 9, 2015

misterdjules removed this from the 0.11.15 milestone Jan 10, 2015

sam-github added 3 commits January 12, 2015 12:07

cluster: don't assume all workers bind same ports

9305d38

For shared handles that do not get connection close messages (UDP/dgram is the only example of this), cluster must not assume that a port listened on by one worker is listened on by all workers.

cluster: cluster.disconnect() should check status

ae037ee

Workers that are already disconnected but not yet exited should not be disconnected, trying to do so raises exceptions.

sam-github mentioned this pull request Jan 13, 2015

dgram: implicit binds should be exclusive nodejs/node#325

Closed

sam-github closed this Feb 17, 2015

sam-github deleted the dont-share-dgram-send-sockets branch February 17, 2015 03:01

jbt added a commit to jbt/node-statsd-client that referenced this pull request Feb 26, 2015

EphemeralSocket should be exclusive

7f58aa0

... otherwise it doesn't play nicely with cluster. Similar: nodejs/node-v0.x-archive#8643

jbt mentioned this pull request Feb 26, 2015

EphemeralSocket should be exclusive msiebuhr/node-statsd-client#45

Merged

This was referenced Feb 27, 2015

AssertionError: false == true at RoundRobinHandle.add (cluster.js:140:3) #9296

Closed

AssertionError: false == true at SharedHandle.add (cluster.js:97:3) #9261

Closed

csimi mentioned this pull request Jul 23, 2015

Exclusive dgram bind to avoid cluster exceptions sivy/node-statsd#58

Closed

d00rman mentioned this pull request Sep 29, 2015

Use bunyan-syslog-udp instead of bunyan-syslog wikimedia/service-runner#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't share dgram sockets that are implicitly bound #8643

Don't share dgram sockets that are implicitly bound #8643

sam-github commented Oct 29, 2014

asilvas commented Oct 29, 2014

rmg commented Oct 29, 2014

sam-github commented Nov 14, 2014

greenboxal commented Nov 15, 2014

sam-github commented Nov 17, 2014

greenboxal commented Nov 17, 2014

sam-github commented Nov 26, 2014

misterdjules commented Nov 26, 2014

sam-github commented Dec 4, 2014

piscisaureus commented Dec 4, 2014

misterdjules commented Dec 5, 2014

sam-github commented Dec 23, 2014

misterdjules commented Dec 24, 2014

sam-github commented Jan 9, 2015

misterdjules commented Jan 9, 2015

misterdjules commented Jan 9, 2015

misterdjules commented Jan 9, 2015

sam-github commented Jan 10, 2015

piscisaureus commented Jan 10, 2015

piscisaureus commented Jan 10, 2015

piscisaureus commented Jan 10, 2015

misterdjules commented Jan 11, 2015

sam-github commented Jan 13, 2015

sam-github commented Jan 13, 2015

misterdjules commented Feb 17, 2015

cjihrig commented Feb 17, 2015

trevnorris commented Feb 18, 2015

misterdjules commented Feb 18, 2015

misterdjules commented Feb 18, 2015

Don't share dgram sockets that are implicitly bound #8643

Don't share dgram sockets that are implicitly bound #8643

Conversation

sam-github commented Oct 29, 2014

asilvas commented Oct 29, 2014

rmg commented Oct 29, 2014

sam-github commented Nov 14, 2014

greenboxal commented Nov 15, 2014

sam-github commented Nov 17, 2014

greenboxal commented Nov 17, 2014

sam-github commented Nov 26, 2014

misterdjules commented Nov 26, 2014

sam-github commented Dec 4, 2014

piscisaureus commented Dec 4, 2014

misterdjules commented Dec 5, 2014

sam-github commented Dec 23, 2014

misterdjules commented Dec 24, 2014

sam-github commented Jan 9, 2015

misterdjules commented Jan 9, 2015

misterdjules commented Jan 9, 2015

misterdjules commented Jan 9, 2015

sam-github commented Jan 10, 2015

piscisaureus commented Jan 10, 2015

piscisaureus commented Jan 10, 2015

piscisaureus commented Jan 10, 2015

misterdjules commented Jan 11, 2015

sam-github commented Jan 13, 2015

sam-github commented Jan 13, 2015

misterdjules commented Feb 17, 2015

cjihrig commented Feb 17, 2015

trevnorris commented Feb 18, 2015

misterdjules commented Feb 18, 2015

misterdjules commented Feb 18, 2015