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.

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

dgram: implicit binds should be exclusive #325

Closed

Conversation

sam-github
Copy link
Contributor

(re)-PR of fix for #279

Passes make test.

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-URL: nodejs/node-v0.x-archive#8643
Reviewed-by: Bert Belder <[email protected]>
@sam-github
Copy link
Contributor Author

/to @piscisaureus @bnoordhuis

Note that since I cherry-picked a32b92d, it has some of Bert's annotations in it, I wasn't sure if I should strip them or leave them, so I left them.

@bnoordhuis
Copy link
Member

LGTM but I would like to keep this open until the next release.

@sam-github
Copy link
Contributor Author

@bnoordhuis @piscisaureus I've only watched half the iojs TC meetings, has there been discussion of how PRs that effect the minor will be handled that you can point me to?

I assume since this is a new feature, it should be labelled minor (as I think was decided a TC meeting ago). If it gets merged, then the next release of iojs will have to be 1.1.0. If it doesn't get merged, I guess a set of minor PRs will accumulate, until the TC decides there is enough it wants to batch merge them all in, and rev the minor.

What is the plan for features, to prevent iojs becoming like node 0.10, where no new features were released for 1.5+ years and counting?

@rvagg
Copy link
Member

rvagg commented Jan 20, 2015

There's a discussion scheduled for tomorrow's TC on some of the issues around this. I think the simple answer for now is that we shouldn't be afraid of server, Node.js has made us sensitive to bumping minor version (coming up 2 years on 0.10) but we need to shake that. However, it would probably be most optimal to group minor changes, perhaps under a v1.1 branch but not delay that too much.

@bnoordhuis
Copy link
Member

I was going to treat this as a bug fix but now that you mention it, I'll bring it up at the TC meeting tomorrow whether this PR constitutes a patch or a minor version bump.

What is the plan for features, to prevent iojs becoming like node 0.10, where no new features were released for 1.5+ years and counting?

I was going to comment on this but @rvagg beat me to it, see above.

@sam-github
Copy link
Contributor Author

OK. I'm listening to the TC Meeting 2014-12-17 meeting now, and I see its under discussion, I might listen to tomorrows meeting live.

Fwiw, I'm fine with batching up minor changes on a branch, or even batching them up as open PRs with label minor and label accepted, or whatever, as long as its clear what the state is, and there is some general guidance on when the release that will incorporate these changes is going to happen.

I'm not particularly concerned about this small thing, but in general, not knowing when a feature is going to be released discourages adding new features. This is particularly true for non-backwards compatible changes, I've a set I'd like to implement, but its hard to know if its worth the effort, will there be willingness for a major version bump in the next 3 months? 6 months? Year?

@rvagg
Copy link
Member

rvagg commented Jan 20, 2015

There's no appetite here for delay, everyone just one wants to use semver to signal and just get on with it. I imagine 1.1.0 to be relatively close.

@rvagg
Copy link
Member

rvagg commented Jan 23, 2015

this was tagged with tc-agenda but I'm not sure we properly covered it, @bnoordhuis your call as to whether the tag is removed or left for next meeting bundled with further versioning discussion

@rvagg rvagg added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 23, 2015
@sam-github
Copy link
Contributor Author

I think the intention was to discuss the versioning implications in TC, not the change itself.

Versioning was discussed, yeah for the semver-* labels!

@rvagg
Copy link
Member

rvagg commented Jan 27, 2015

#625 being merged has forced us to a semver-minor situation so this could probably be merged now so it makes it in to 1.1.0

piscisaureus pushed a commit 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 <[email protected]>
Reviewed-by: Bert Belder <[email protected]>
@piscisaureus
Copy link
Contributor

Thanks, landed in 65b1e4f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants