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

test: remove an unnecessary call to bind and related comments #5023

Closed
wants to merge 1 commit into from

Conversation

aayn
Copy link
Contributor

@aayn aayn commented Feb 1, 2016

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

As mentioned in the comment of the changed file, "a libuv limitation
makes it necessary to bind()". But, that is not the case anymore. So,
it(meaning 'bind()') can be removed.
@ChALkeR ChALkeR added the test Issues and PRs related to the tests. label Feb 1, 2016
@mscdex mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label Feb 1, 2016
@jasnell
Copy link
Member

jasnell commented Feb 1, 2016

@saghul @bnoordhuis

@saghul
Copy link
Member

saghul commented Feb 2, 2016

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
Copy link
Contributor Author

aayn commented Feb 2, 2016

@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
Copy link
Member

saghul commented Feb 2, 2016

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
Copy link
Contributor Author

aayn commented Feb 2, 2016

@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
Copy link
Member

saghul commented Feb 2, 2016

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
Copy link
Contributor Author

aayn commented Feb 3, 2016

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
Copy link
Member

Trott commented Feb 27, 2016

@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.)

@Trott Trott mentioned this pull request Feb 27, 2016
11 tasks
@saghul
Copy link
Member

saghul commented Feb 28, 2016

@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
Copy link
Member

Trott commented Feb 28, 2016

@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
Copy link
Member

saghul commented Feb 29, 2016

@Trott you are 100% correct 👍

@Trott
Copy link
Member

Trott commented Feb 29, 2016

@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
Copy link
Member

saghul commented Feb 29, 2016

@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
Copy link
Member

Trott commented Feb 29, 2016

@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
Copy link
Member

saghul commented Feb 29, 2016

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
Copy link
Member

Trott commented Feb 29, 2016

@Trott
Copy link
Member

Trott commented Mar 1, 2016

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

Trott pushed a commit to Trott/io.js that referenced this pull request Mar 1, 2016
As mentioned in the comment of the changed file, "a libuv limitation
makes it necessary to bind()". But, that is not the case in this test.
The subsequent call to send() results in an implicit bind().

PR-URL: nodejs#5023
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@Trott
Copy link
Member

Trott commented Mar 1, 2016

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.)

@Trott Trott closed this Mar 1, 2016
@Fishrock123 Fishrock123 mentioned this pull request Mar 1, 2016
5 tasks
Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
As mentioned in the comment of the changed file, "a libuv limitation
makes it necessary to bind()". But, that is not the case in this test.
The subsequent call to send() results in an implicit bind().

PR-URL: #5023
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
As mentioned in the comment of the changed file, "a libuv limitation
makes it necessary to bind()". But, that is not the case in this test.
The subsequent call to send() results in an implicit bind().

PR-URL: #5023
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
As mentioned in the comment of the changed file, "a libuv limitation
makes it necessary to bind()". But, that is not the case in this test.
The subsequent call to send() results in an implicit bind().

PR-URL: #5023
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants