test: remove an unnecessary call to bind and related comments

[aayn]

Ref: #4640.

As mentioned in the above issue, some of the files have TODO, FIXME and XXX comments which should be removed. This is one of them. The comment(which is to be removed) says, "a libuv limitation makes it necessary to bind()". But, that is not the case any more. So, it can be removed.

/cc @Trott @Fishrock123

[jasnell]

@saghul @bnoordhuis

[saghul]

This is incorrect. It's true that now sockets can be created early by using uv_udp_init_ex(loop, handle, af), but Node doesn't do that: https://github.com/nodejs/node/blob/master/src/udp_wrap.cc#L70

AFAIS we already know the socket family when the handle is created: https://github.com/nodejs/node/blob/master/lib/dgram.js#L41 so creating the socket early would make some sense here. That way we can fail early in case of EMFILE, and not in bind(), which can be unexpected.

Now, that implies that the constructor will have to throw here: https://github.com/nodejs/node/blob/master/src/udp_wrap.cc#L71 so some handling would be required, and would probably warrant a semver-major.

Feel free to @ me for review if you go that route.

[aayn]

@saghul Oh okay. I understood some parts of what you said, but I'm kinda new here(my first contribution), so could you please explain in a bit more detail(what 'handling' are you referring to?), or at least point me to some resources? Thanks 😄 .

[saghul]

No problem Aayush.

Right now the UDPWrap constructor cannot fail. It asserts that the return value of uv_udp_init is 0 here.

Now, if you change it so the constructor gets the address family (AF_INET or AF_INET6) we would use uv_udp_init_ex, which creates the socket for the given family. This could fail if the system is out of file descriptors (UV_EMFILE) so the constructor would have to throw.

Then that would mean that this line can throw, so that needs to be handled somehow. The problem I see is that dgram.createSocket doesn't take an errback, I guess because there was no way this could fail.

As you can see, a few things would need to change. It's probably best to come up with a proposal before jumping in all the way, there might be dragons! :-)

[aayn]

@saghul So, this is what I understood so far(please correct me if I'm wrong), I need to replace uv_udp_init from here, with uv_udp_init_ex having the necessary flags(AF_INET or AF_INET6) as documented here.

One question: what does uv_udp_init_ex return when it succeeds(or fails)?

Then, based on its return value, make a check here.
After that, add an error callback to dgram.createSocket.

[saghul]

One question: what does uv_udp_init_ex return when it succeeds(or fails)?

It returns 0 on success or an UV error in case of failure.

The rest is correct, but I'd wait for some more opinions before jumping in.

[aayn]

Alright, cool. So, I'll wait for some time before diving in. Also, I have exams in 3 days, so I'll be inactive for a while. After that I'll be ready for whatever comes my way.

[Trott]

@saghul Am I mistaken about the following analysis?:

• dgram.setTTL() in JS-land is basically uv_udp_set_ttl().
• uv_udp_set_ttl() requires a call to uv_udp_init() before it can be used
• By the time the listening event fires on the socket, Node.js is guaranteed to have called uv_udp_init() so we don't need the sendSocket.bind() call in this test. We probably did at some point in the past but not anymore.

Empirically, the test passes (on OS X, anyway) if you remove the bind() call. The output remains nearly identical. (PIDs change, of course.)

[saghul]

@Trott calling uv_udp_set_ttl requires that the socket actually exists. When uv_udp_init is called the socket is not created, because we do that lazily. When uv_udp_init_ex is called with AF_INET or AF_INET6, the socket is created on the spot. If you call it with AF_UNSPEC it's the same as just doing uv_udp_init, that is, the socket is created lazily.

So, if you call uv_udp_set_ttl before the socket is created you'll get UV_EBADF. Calling uv_udp_bind will create the socket if it wasn't created yet.

Now, as I mentioned here, it seems like we already know the address family when creating the handle, hence my suggestion for switching to using uv_udp_init_ex. That however introduces a new point where an error could be produced (uv_udp_init can never fail) so it needs to be handled.

[Trott]

@saghul So, if I'm understanding everything correctly, this would be the state of affairs:

• It is currently possible to call setTTL() on a dgram socket in Node before the socket is actually ready to have its TTL set because the socket is created lazily. Doing this results in Node throwing an error (setTTL EBADF).
• You have explained how that issue might be resolved by using uv_udp_init_ex under the hood rather than uv_udp_init.
• However, for this specific test file, the calls to setTTL() and the other set* functions are actually safe without the bind() because they are inside the listening callback, by which time the socket must exist. Therefore the bind() call in this specific test file is not needed. If the setTTL() is moved outside the listening callback, then we get the expected EBADF error. But as long as it is inside the listening callback, it should be OK. (Or is that TOTALLY WRONG because the behavior is OS-dependent or something like that?)

[saghul]

@Trott you are 100% correct 👍

[Trott]

@saghul Great! Thanks for your patience explaining this.

Unless there's a reason not to, I'm going to give this PR an LGTM and open a separate issue to discuss/address the possibility of a generalized fix as you've outlined it here.

[saghul]

@Trott thanks for opening the issue! As for this PR, does the test pass? I don't see how the socket can emit 'listening' without calling bind or any other function which causes an implicit one.

[Trott]

@saghul It passes for me on OS X and the callback is definitely firing. I would expect an implicit .bind() via the subsequent .send().

[saghul]

Ah, right you are. LGTM.

On Mon, Feb 29, 2016 at 11:04 PM, Rich Trott notifications@github.com
wrote:

@saghul https://github.com/saghul It passes for me on OS X and the
callback is definitely firing. I would expect an implicit .bind() via the
subsequent .send()
https://github.com/aayn/node/blob/master/test/internet/test-dgram-multicast-multi-process.js#L156-L169
.

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

/Saúl
bettercallsaghul.com

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/1784/

[Trott]

One CI infrastructure problem, but CI is good other than that.

[Trott]

Landed in 93bacfd. Thanks, @aayn!

(I edited the commit message to reflect the facts as they came out in this conversation. I also had to edit the first line to get it under the 50-character maximum for the first line of commit messages.)