-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Conversation
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.
This is incorrect. It's true that now sockets can be created early by using 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. |
@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 😄 . |
No problem Aayush. Right now the Now, if you change it so the constructor gets the address family ( Then that would mean that this line can throw, so that needs to be handled somehow. The problem I see is that 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! :-) |
@saghul So, this is what I understood so far(please correct me if I'm wrong), I need to replace One question: what does Then, based on its return value, make a check here. |
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. |
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. |
@saghul Am I mistaken about the following analysis?:
Empirically, the test passes (on OS X, anyway) if you remove the |
@Trott calling So, if you call 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 |
@saghul So, if I'm understanding everything correctly, this would be the state of affairs:
|
@Trott you are 100% correct 👍 |
@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. |
@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 |
@saghul It passes for me on OS X and the callback is definitely firing. I would expect an implicit |
Ah, right you are. LGTM. On Mon, Feb 29, 2016 at 11:04 PM, Rich Trott notifications@github.com
/Saúl |
One CI infrastructure problem, but CI is good other than that. |
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>
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>
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>
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>
Ref: #4640.
As mentioned in the above issue, some of the files have
TODO
,FIXME
andXXX
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