Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

sam-github
Copy link

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
Copy link

asilvas commented Oct 29, 2014

+1

@rmg
Copy link

rmg commented Oct 29, 2014

👍 each commit is a one-liner + tests.

@sam-github
Copy link
Author

@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
Copy link

+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
Copy link
Author

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
Copy link

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
Copy link
Author

Will fix #5587 if this lands.

@misterdjules
Copy link

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

@sam-github
Copy link
Author

@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
Copy link

LGTM

@misterdjules
Copy link

@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
Copy link
Author

@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
Copy link

@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
Copy link
Author

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

@misterdjules
Copy link

@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
Copy link

@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
Copy link

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
Copy link
Author

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

piscisaureus pushed a commit to nodejs/node that referenced this pull request Jan 10, 2015
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 <bertbelder@gmail.com>
@piscisaureus
Copy link

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

@piscisaureus
Copy link

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

@piscisaureus
Copy link

@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 misterdjules removed this from the 0.11.15 milestone Jan 10, 2015
@misterdjules
Copy link

@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.
sam-github added a commit to sam-github/node that referenced this pull request Jan 13, 2015
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 <bertbelder@gmail.com>
@sam-github
Copy link
Author

Fixed in 0b827a0

@sam-github
Copy link
Author

cf. nodejs/node#325

piscisaureus pushed a commit to nodejs/node that referenced this pull request Jan 14, 2015
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 <bertbelder@gmail.com>
piscisaureus pushed a commit to nodejs/node that referenced this pull request Jan 30, 2015
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 <info@bnoordhuis.nl>
Reviewed-by: Bert Belder <bertbelder@gmail.com>
@sam-github sam-github closed this Feb 17, 2015
@sam-github sam-github deleted the dont-share-dgram-send-sockets branch February 17, 2015 03:01
@misterdjules
Copy link

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

@cjihrig
Copy link

cjihrig commented Feb 17, 2015

+1 from me

@trevnorris
Copy link

@misterdjules +1

@misterdjules
Copy link

@cjihrig @trevnorris Thanks! Will land it asap.

misterdjules pushed a commit to misterdjules/node that referenced this pull request Feb 18, 2015
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 <info@bnoordhuis.nl>
Reviewed-by: Bert Belder <bertbelder@gmail.com>
Reviewed-by: Julien Gilli <julien.gilli@joyent.com>
misterdjules pushed a commit that referenced this pull request Feb 18, 2015
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 <info@bnoordhuis.nl>
Reviewed-by: Bert Belder <bertbelder@gmail.com>
Reviewed-by: Julien Gilli <julien.gilli@joyent.com>
@misterdjules
Copy link

Landed in e42c4a3. Thanks again @sam-github!

jbt added a commit to jbt/node-statsd-client that referenced this pull request Feb 26, 2015
... otherwise it doesn't play nicely with cluster. 

Similar: nodejs/node-v0.x-archive#8643
jasnell pushed a commit to jasnell/node-joyent that referenced this pull request Mar 14, 2015
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 <info@bnoordhuis.nl>
Reviewed-by: Bert Belder <bertbelder@gmail.com>
Reviewed-by: Julien Gilli <julien.gilli@joyent.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants