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: default send address to 127.0.0.1 or ::1 #5493

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@mcollina
Member

mcollina commented Feb 29, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

dgram

Description of change

In net we default to 'localhost' as the default address for connect.
Not doing the same on dgram is confusing, because sending to 0.0.0.0
works on Linux/OS X but not on Windows. Defaulting that to 127.0.0.1 /
::1 addresses that.

Related: #5407
Related: #5398

Fixes: #5487

cc @mafintosh @rvagg @cjihrig @saghul @feross @silverwind

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Feb 29, 2016

Contributor

This does include a bind change too, is that intentional? I'd suggest doing that in a separate PR.

Contributor

silverwind commented Feb 29, 2016

This does include a bind change too, is that intentional? I'd suggest doing that in a separate PR.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Feb 29, 2016

Member

@silverwind yes. Both send and bind use the same defaults in the lookup functions: they need to be different. I thought to include the additional ifs in the bind call, given the fact that send is a hot path.

Member

mcollina commented Feb 29, 2016

@silverwind yes. Both send and bind use the same defaults in the lookup functions: they need to be different. I thought to include the additional ifs in the bind call, given the fact that send is a hot path.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Feb 29, 2016

Contributor

Ah, alright, misread that change. I think you can do '::' instead of '::0' by the way.

Contributor

silverwind commented Feb 29, 2016

Ah, alright, misread that change. I think you can do '::' instead of '::0' by the way.

@mafintosh

This comment has been minimized.

Show comment
Hide comment
@mafintosh

mafintosh Feb 29, 2016

Member

before re-reading the docs last week i thought it already worked like this so big +1 from me.

Member

mafintosh commented Feb 29, 2016

before re-reading the docs last week i thought it already worked like this so big +1 from me.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Feb 29, 2016

Member

Since this will be a semver-major, can we make the address in send mandatory?

Member

saghul commented Feb 29, 2016

Since this will be a semver-major, can we make the address in send mandatory?

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Feb 29, 2016

Contributor

can we make the address in send mandatory?

I'm not opposed to this, but if we do that, we need to do it in net too where it's also optional.

Contributor

silverwind commented Feb 29, 2016

can we make the address in send mandatory?

I'm not opposed to this, but if we do that, we need to do it in net too where it's also optional.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Feb 29, 2016

Member

@mafintosh do you think that defining it as semver-major is too strict? Given that's how people use this, should it be semver-minor?

@saghul @silverwind I think those needs to be two separate fixes. To remove the default from net and dgram would cause a lot of modules/applications tests to start failing. I'm ok for removing the default in v7/v8, but not in v6 as it will be an LTS (and a lot of people will jump on that when it comes out, given that it will be "the" LTS version).

Member

mcollina commented Feb 29, 2016

@mafintosh do you think that defining it as semver-major is too strict? Given that's how people use this, should it be semver-minor?

@saghul @silverwind I think those needs to be two separate fixes. To remove the default from net and dgram would cause a lot of modules/applications tests to start failing. I'm ok for removing the default in v7/v8, but not in v6 as it will be an LTS (and a lot of people will jump on that when it comes out, given that it will be "the" LTS version).

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Feb 29, 2016

Member

I'm not opposed to this, but if we do that, we need to do it in net too where it's also optional.

There is no send in net. Stream sockets are connected, so the target is already "fixed", aka, the other side. In datagram sockets, you can select the target for each datagram. They are different and they already behave different.

Member

saghul commented Feb 29, 2016

I'm not opposed to this, but if we do that, we need to do it in net too where it's also optional.

There is no send in net. Stream sockets are connected, so the target is already "fixed", aka, the other side. In datagram sockets, you can select the target for each datagram. They are different and they already behave different.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Feb 29, 2016

Member

I'm not opposed to this, but if we do that, we need to do it in net too where it's also optional.

There is no send in net. Stream sockets are connected, so the target is already "fixed", aka, the other side. In datagram sockets, you can select the target for each datagram. They are different and they already behave different.

There is net.connect(port [, address]), which defaults to localhost. IMHO, having a default for address in net.connect() is really similar to one for socket.send in dgram.

Member

mcollina commented Feb 29, 2016

I'm not opposed to this, but if we do that, we need to do it in net too where it's also optional.

There is no send in net. Stream sockets are connected, so the target is already "fixed", aka, the other side. In datagram sockets, you can select the target for each datagram. They are different and they already behave different.

There is net.connect(port [, address]), which defaults to localhost. IMHO, having a default for address in net.connect() is really similar to one for socket.send in dgram.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Feb 29, 2016

Member

Ouch. Yeah, IMHO that should be mandatory too. The only place where it's ok
for it to be optional is bind, I'd say.

On Mon, Feb 29, 2016 at 10:59 PM, Matteo Collina notifications@github.com
wrote:

I'm not opposed to this, but if we do that, we need to do it in net too
where it's also optional.

There is no send in net. Stream sockets are connected, so the target is
already "fixed", aka, the other side. In datagram sockets, you can select
the target for each datagram. They are different and they already behave
different.

There is net.connect(port [, address]), which defaults to localhost.
IMHO, having a default for address in net.connect() is really similar to
one for socket.send in dgram.


Reply to this email directly or view it on GitHub
#5493 (comment).

/Saúl
bettercallsaghul.com

Member

saghul commented Feb 29, 2016

Ouch. Yeah, IMHO that should be mandatory too. The only place where it's ok
for it to be optional is bind, I'd say.

On Mon, Feb 29, 2016 at 10:59 PM, Matteo Collina notifications@github.com
wrote:

I'm not opposed to this, but if we do that, we need to do it in net too
where it's also optional.

There is no send in net. Stream sockets are connected, so the target is
already "fixed", aka, the other side. In datagram sockets, you can select
the target for each datagram. They are different and they already behave
different.

There is net.connect(port [, address]), which defaults to localhost.
IMHO, having a default for address in net.connect() is really similar to
one for socket.send in dgram.


Reply to this email directly or view it on GitHub
#5493 (comment).

/Saúl
bettercallsaghul.com

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 1, 2016

Member

I would expect test/parallel/test-dgram-send-default-host.js to fail linting for removing the timer declaration but keeping a use of timer elsewhere in the test.

Member

Trott commented Mar 1, 2016

I would expect test/parallel/test-dgram-send-default-host.js to fail linting for removing the timer declaration but keeping a use of timer elsewhere in the test.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Mar 2, 2016

Member

@Trott no, lint passes. Good spot, I've updated the tests.

@saghul I agree with you that net.connect() and dgram.send() should not had a default to localhost in the first place. However, everybody uses those defaults, so it will cause a major disruption, and I am not sure it is worth it in the first place. I'm generally ok in such a change, but the impact on the community needs to be evaluated (not sure we have the means though).

Can I get a bunch of LGTM (or comments) so we land this?

Also, given that the intended behavior of send would have been to send the message to localhost anyway, can we consider this semver-minor?

Member

mcollina commented Mar 2, 2016

@Trott no, lint passes. Good spot, I've updated the tests.

@saghul I agree with you that net.connect() and dgram.send() should not had a default to localhost in the first place. However, everybody uses those defaults, so it will cause a major disruption, and I am not sure it is worth it in the first place. I'm generally ok in such a change, but the impact on the community needs to be evaluated (not sure we have the means though).

Can I get a bunch of LGTM (or comments) so we land this?

Also, given that the intended behavior of send would have been to send the message to localhost anyway, can we consider this semver-minor?

@saghul

View changes

Show outdated Hide outdated lib/dgram.js Outdated
@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Mar 2, 2016

Member

LGTM with a bit.

Member

saghul commented Mar 2, 2016

LGTM with a bit.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Mar 2, 2016

Contributor

LGTM. Regarding semver: what does a send to 0.0.0.0 actually do when you have multiple interfaces? Does it send/receive a datagram on each?

Contributor

silverwind commented Mar 2, 2016

LGTM. Regarding semver: what does a send to 0.0.0.0 actually do when you have multiple interfaces? Does it send/receive a datagram on each?

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Mar 2, 2016

Member

The behavior that I am seeing is: it just picks one of the interfaces on Linux and OS X, normally localhost. On Windows nothing is received.

Member

mcollina commented Mar 2, 2016

The behavior that I am seeing is: it just picks one of the interfaces on Linux and OS X, normally localhost. On Windows nothing is received.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Mar 2, 2016

Contributor

I'd say this is patch level commit if it's been going to localhost on most OSs.

Contributor

silverwind commented Mar 2, 2016

I'd say this is patch level commit if it's been going to localhost on most OSs.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina
Member

mcollina commented Mar 3, 2016

@mcollina

This comment has been minimized.

Show comment
Hide comment
@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Mar 3, 2016

Contributor

@mcollina did you check that CERTIFY_SAFE box? See nodejs/build#342 (comment)

Contributor

silverwind commented Mar 3, 2016

@mcollina did you check that CERTIFY_SAFE box? See nodejs/build#342 (comment)

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina
Member

mcollina commented Mar 4, 2016

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Mar 4, 2016

Member

Skipping the dgram6 tests if ipv6 is not supported: https://ci.nodejs.org/job/node-test-pull-request/1846/.

Member

mcollina commented Mar 4, 2016

Skipping the dgram6 tests if ipv6 is not supported: https://ci.nodejs.org/job/node-test-pull-request/1846/.

dgram: default send address to 127.0.0.1 or ::1
In net we default to 'localhost' as the default address for connect.
Not doing the same on dgram is confusing, because sending to 0.0.0.0
works on Linux/OS X but not on Windows. Defaulting that to 127.0.0.1 /
::1 addresses that.

Related: #5407
Related: #5398

Fixes: #5487
@mcollina

This comment has been minimized.

Show comment
Hide comment
Member

mcollina commented Mar 4, 2016

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Mar 4, 2016

Member

Thanks @silverwind, that did the trick.

Any other opinion on the semver level? I think semver-minor or even patch are good: the behavior we are changing is broken in the affected OS. I'm changing this to patch, and I propose we don't land this in LTS anyway (just in case).

Any other opinion, lgtm or anything else before I land?

Member

mcollina commented Mar 4, 2016

Thanks @silverwind, that did the trick.

Any other opinion on the semver level? I think semver-minor or even patch are good: the behavior we are changing is broken in the affected OS. I'm changing this to patch, and I propose we don't land this in LTS anyway (just in case).

Any other opinion, lgtm or anything else before I land?

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Mar 5, 2016

Member

I see two failed tests on OSX, but I can't open them https://ci.nodejs.org/job/node-test-commit-osx/2500/nodes=osx1010/

Member

mcollina commented Mar 5, 2016

I see two failed tests on OSX, but I can't open them https://ci.nodejs.org/job/node-test-commit-osx/2500/nodes=osx1010/

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Mar 5, 2016

Contributor

Getting gateway timeouts too. The failures look unrelated, here they are from the plaintext output:

not ok 1049 test-stringbytes-external-exceed-max.js
# TIMEOUT
  ---
  duration_ms: 61.404

not ok 1053 test-stringbytes-external-exceed-max-by-1-hex.js
# TIMEOUT
  ---
  duration_ms: 60.940
Contributor

silverwind commented Mar 5, 2016

Getting gateway timeouts too. The failures look unrelated, here they are from the plaintext output:

not ok 1049 test-stringbytes-external-exceed-max.js
# TIMEOUT
  ---
  duration_ms: 61.404

not ok 1053 test-stringbytes-external-exceed-max-by-1-hex.js
# TIMEOUT
  ---
  duration_ms: 60.940
@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Mar 5, 2016

Member

Landed in 8af4bb8.

Member

mcollina commented Mar 5, 2016

Landed in 8af4bb8.

@mcollina mcollina closed this Mar 5, 2016

mcollina added a commit that referenced this pull request Mar 5, 2016

dgram: default send address to 127.0.0.1 or ::1
In net we default to 'localhost' as the default address for connect.
Not doing the same on dgram is confusing, because sending to 0.0.0.0
works on Linux/OS X but not on Windows. Defaulting that to 127.0.0.1 /
::1 addresses that.

Related: #5407
Related: #5398
Fixes: #5487
PR-URL: #5493
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

@Fishrock123 Fishrock123 referenced this pull request Mar 7, 2016

Merged

v5.8.0 proposal #5559

Fishrock123 added a commit that referenced this pull request Mar 8, 2016

dgram: default send address to 127.0.0.1 or ::1
In net we default to 'localhost' as the default address for connect.
Not doing the same on dgram is confusing, because sending to 0.0.0.0
works on Linux/OS X but not on Windows. Defaulting that to 127.0.0.1 /
::1 addresses that.

Related: #5407
Related: #5398
Fixes: #5487
PR-URL: #5493
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

Fishrock123 added a commit that referenced this pull request Mar 8, 2016

dgram: default send address to 127.0.0.1 or ::1
In net we default to 'localhost' as the default address for connect.
Not doing the same on dgram is confusing, because sending to 0.0.0.0
works on Linux/OS X but not on Windows. Defaulting that to 127.0.0.1 /
::1 addresses that.

Related: #5407
Related: #5398
Fixes: #5487
PR-URL: #5493
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment