dgram: instantiate socket on creation

[Trott]

In Node 5.7.0 and all currently-supported previous versions, it is possible to try to trigger an exception (EBADF) with code such as this:

var dgram = require('dgram');

var socket = dgram.createSocket('udp4');
socket.setTTL(1);

As @saghul explains in #5023, this is because dgram instantiates the socket lazily with uv_udp_init(). If uv_udp_init_ex() were used instead, then the socket could be created and ready for the various .set* functions upon creation.

Such a change would probably be semver-major because createSocket() might throw whereas currently that is not the case.

There are probably loads of other dangers and considerations. (I'm opening this issue so that the discussion can be had independently of the PR linked above, which is a relatively minor and safe change to a test file. This, on the other hand, would be a much bigger deal...)

[mscdex]

What about lazily setting those options like what is currently done with net.Socket and socket.setKeepAlive(), and socket.setNoDelay()?

[saghul]

That would be unnecessary. An interesting bonus is that if the system runs
out of file descriptors it blows up on socket creation, not in some weird
place like bind.

On Tue, Mar 1, 2016 at 12:20 AM, Brian White notifications@github.com
wrote:

What about lazily setting those options like what is currently done with
net.Socket and socket.setKeepAlive(), and socket.setNoDelay()?

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

/Saúl
bettercallsaghul.com

[jasnell]

@Trott... does this need to stay open?

[Trott]

@jasnell AFAIK, it should stay open, but the expert here is @saghul so let's ask them.

[cjihrig]

Making this change seems to be more involved than I originally thought. We would now need to pass the socket type to UDPWrap::UDPWrap(). It works fine in the normal case, but blows up when invoked via UDPWrap::Instantiate(). If it's not possible to pass a value through, then we could default to AF_UNSPEC, but that would just give us the existing behavior. Maybe @indutny could comment.

[Trott]

I'm going to close this as "sure, could be improved, but not really a bug, it's async, that's the way it is"... But feel free to re-open or comment if you think that's the wrong move.