-
Notifications
You must be signed in to change notification settings - Fork 29k
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
net: rework autoSelectFamily implementation #46587
net: rework autoSelectFamily implementation #46587
Conversation
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the performance impact of this pull request?
Putting on the tsc-agenda for visibility. |
I'll review comments on Mon. This will involve a C++ change that I tried but I'm not sure if it has side effects. How can I ping for help on that? |
cc @nodejs/cpp-reviewers |
Thanks @anonrig!
Changes pushed. @nodejs/cpp-reviewers @nodejs/net To simplify and stabilize the implementation I introduce a new "Reinitialize" method on the TCP wrap to create a new uv_tcp_t. This is better that then previous implementation I had that swapped TCPWrap after connecting since it has less side effects. But do you think the C++ is valid? Do you think it can have any side effect? |
@ShogunPanda I would split the C++ change from flipping the default into two different PRs, I think the C++ changes do not need to be semver-major. |
You mean C++ changes first and then I flip the switch? |
I think we should backport the fix, but not the switch of the default. Right now they would both be set as semver-major. |
f4ffcb8
to
c8b5f22
Compare
src/tcp_wrap.cc
Outdated
&wrap, args.Holder(), args.GetReturnValue().Set(UV_EBADF)); | ||
|
||
Environment* env = wrap->env(); | ||
int r = uv_tcp_init(env->event_loop(), &wrap->handle_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have some sort of check like this?
int r = uv_tcp_init(env->event_loop(), &wrap->handle_); | |
CHECK(!IsAlive(this)); | |
int r = uv_tcp_init(env->event_loop(), &wrap->handle_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, probably. I'll add this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax I tried, and it always fails, both in TCPWrap::TCPWrap
or in TCPWrap::Reinitialize
. So I guess this is not needed. Or am I doing the wrong check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that the handle is not closed at this point? I don’t think it’s safe to call uv_..._init()
functions on open handles? @bnoordhuis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handle just received uv_tcp_init
but never any open/connect methods. Does it still mean they are considered opened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pointer to the uv_tcp_t
object needs to stay alive until uv_close()
is done with it. At the very least you'd need to pass a callback that frees the memory afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is fine. I can definetely do that.
Memory problems aside, do you foresee any problems in swapping the handler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in theory this works, but it’s going to be a bit more complex than the code above.
For example, you’ll need to update all ConnectionWrap
subclasses to have a pointer handle member rather than a plain member, and you’re going to have to use Environment::CloseHandle()
rather than uv_close()
so that the close callback gets tracked properly.
My gut feeling would be that at that point, it probably makes more sense to look into how to handle this more properly on the JS layer rather than trying to re-use TCPWrap
instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, you are probably right, I'm looking for too much troubles. I'll see how to fix it on the JS side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax I finally solved (hopefully all) the issues without touching the C++ and swapping handles.
Thanks for not having me kill with my own hands :)
The ASan failures are relevant, in particular, |
873bbcc
to
fe56de9
Compare
fe56de9
to
8e3a06c
Compare
PR-URL: #46587 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol After doublecheck with @mhdawson we agreed that this PR should indeed land on 18 as it is the foundation to fix the initially faulty network family selection feature. |
I have a backported version of this. @juanarbol should I create a separate backport PR or would it makes sense to be part of #48275? The backport I have depends on 48275 landing first. I also want to backport #47029 and |
@ShogunPanda mentioned that we might also want to include 48464 on the ones backported. I'll add that to my list above. |
You can push into my PR if you need to, I'm ok with whatever you prefer to do |
When I tried to land this, I found that this was using at least one change made by #46790 |
Yeah, but I think @mhdawson resolved those conflicts, isn't it? |
@juanarbol, @ShogunPanda correct I had a set of merged/tweaked PRs including this one. Thinking about it more I assume pushing to the existing PR does not makes sense I'll need additional backport PRs. Will start doing that. |
PR-URL: nodejs#46587 Backport-PR-URL: Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol, @ShogunPanda, @ruyadorno I submitted this PR - #49016 to backport this PR as part of the backport needed to land #48969. It includes the commits from #48275 so it can either be landed after that PR lands or the commits could be merged into that PR for landing. @ruyadorno hoping as the releaser for the nex 18.x release you can decide what is best. |
PR-URL: nodejs#46587 Backport-PR-URL: Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Backport to 18.x PR (second try) - #49183 |
PR-URL: nodejs#46587 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This PR reworks the implementation of
autoSelectFamily
innet.connect
to better swap internal TCPWrap when the connection is successful. It also fixes several issues with TLS.It also add a global getter and setter
setDefaultAutoSelectFamilyAttemptTimeout
andgetDefaultAutoSelectFamilyAttemptTimeout
to allow global customization of the entire family autoselection algorithm.Fixes #46669.
Fixes #46679.