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: follow the same standard of net for the default address #5487

Closed
mcollina opened this issue Feb 29, 2016 · 8 comments
Closed

dgram: follow the same standard of net for the default address #5487

mcollina opened this issue Feb 29, 2016 · 8 comments
Labels
dgram Issues and PRs related to the dgram subsystem / UDP.

Comments

@mcollina
Copy link
Member

In dgram, if you omit the address in send (https://nodejs.org/api/dgram.html#dgram_socket_send_msg_offset_length_port_address_callback), message is sent to '0.0.0.0' or '::0' whereas in net the default address for net.connect is 'localhost' (https://nodejs.org/api/net.html#net_net_connect_options_connectlistener).

This behavior has been around since forever, making dgram.send default address not reliable (see #5407 (comment)).

I propose to reconcile this two behavior, having dgram default to 'localhost' as well.

This will be a semver-major change.

cc @silverwind @rvagg @mafintosh @feross

@mcollina mcollina added the dgram Issues and PRs related to the dgram subsystem / UDP. label Feb 29, 2016
@rvagg
Copy link
Member

rvagg commented Feb 29, 2016

+1 for consistency, no opinion on which one is more correct, if you don't get any more comments here just move forward with a PR, that'll make sure any concerns come out

@silverwind
Copy link
Contributor

+1, sending to 0.0.0.0 is not what I would expect. Why doesn't it work on Windows, though?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 29, 2016

+1 to using 'localhost'. IIRC, net was changed to localhost to avoid problems related to IPv4 vs. IPv6.

@mcollina
Copy link
Member Author

@cjihrig when that was changed? Have you got a PR/issue so I can have a look around?

@silverwind not exactly sure, @saghul knows better.

@feross
Copy link
Contributor

feross commented Feb 29, 2016

+1. I can't think of any reason to keep this inconsistent.

@saghul
Copy link
Member

saghul commented Feb 29, 2016

@mcollina I have no idea why it doesn't :-S Now, I think there are 2 different things here:

  • dgram.bind: this should bind to localhost
  • dgram.send: this should have the address as mandatory, IMHO (except if we introduce connected UDP sockets, which we don't have right now)

@cjihrig
Copy link
Contributor

cjihrig commented Feb 29, 2016

@mcollina sorry, I can't find an exact commit.

@mcollina
Copy link
Member Author

Pr sent #5493

mcollina added a commit to mcollina/node that referenced this issue Mar 4, 2016
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: nodejs#5407
Related: nodejs#5398

Fixes: nodejs#5487
Fishrock123 pushed a commit that referenced this issue Mar 8, 2016
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 pushed a commit that referenced this issue Mar 8, 2016
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
Labels
dgram Issues and PRs related to the dgram subsystem / UDP.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants