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

[Android] Added Happy Eyeballs for address selection #33045

Closed

Conversation

marcesengel
Copy link
Contributor

Summary

PR #32889 to main.

Changelog

[Android] [Feature] - Added Happy Eyeballs for network request target address selection

Test Plan

See #32889.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 4, 2022
@cortinico
Copy link
Contributor

Thanks for opening this PR @marcesengel
I believe we should not introduce an Happy Eyeball implementation inside React Native at this stage.

Square's OkHTTP are going to implement Fast fallback in OkHTTP 5.x, which is their implementation of Happy Eyeball:
https://github.com/square/okhttp/blob/master/CHANGELOG.md#version-500-alpha4

We should rather:

  • Look into bumping our OkHTTP version to the latest stable.
  • Potentially try to adapt our codebase to make sure that is OkHTTP 5x compatible. That means that you'll be able to just bump the OkHTTP version to 5.x without having to make any changes or fork RN to make it work.

@marcesengel
Copy link
Contributor Author

Sounds good @cortinico, I wasn't sure how feasible it would be to wait for a major bump of OkHTTP and to introduce it here.

Closing this in favor of the aforementioned procedure 👍.

In the meantime people can feel free to use my patched branch marcesengel/react-native#0.66-patched-3.

@marcesengel marcesengel closed this Feb 4, 2022
@cortinico
Copy link
Contributor

In the meantime people can feel free to use my patched branch marcesengel/react-native#0.66-patched-3.

Thanks for suggesting this 👍

In the meanwhile, would you be up for opening a draft PR with the OkHTTP bump to 5.x alpha? It could help us spot early breaking changes and we could test it against the internal infra to make sure nothing breaks.
(That's also a task that someone else in the community can pickup if they wish).

@marcesengel
Copy link
Contributor Author

@cortinico some interesting updates on the matter: square/okhttp#506 (comment), seems like reddit is using OkHTTP 5.0.4 in production and it's going smoothly.

Sadly last time I checked I couldn't build RN on my M1 machine, so unless this changed I won't be able to contribute anything anytime soon :(

@cortinico
Copy link
Contributor

Sadly last time I checked I couldn't build RN on my M1 machine, so unless this changed I won't be able to contribute anything anytime soon :(

You should now be able without any problems.

seems like reddit is using OkHTTP 5.0.4 in production and it's going smoothly.

They're still using the alpha which is considered "good enough" for production.

@marcesengel
Copy link
Contributor Author

You should now be able without any problems.

😍

They're still using the alpha which is considered "good enough" for production.

Yup - this ready like the React team won't make this move unless it's stable? Would be a shame imo, as this really breaks stuff for a lot of consumers.

@cortinico
Copy link
Contributor

Would be a shame imo, as this really breaks stuff for a lot of consumers.

What is preventing a consumer to bump OkHTTP version to whatever they wish?

@marcesengel
Copy link
Contributor Author

What is preventing a consumer to bump OkHTTP version to whatever they wish?

Sorry, seems like I worded this poorly. I meant to say that it'd be a shame if the React team doesn't want to make the move to 5.0.4 [because it's alpha], as NOT having 5.0.4 breaks stuff for a lot of consumers [of react-native powered apps].

@cortinico
Copy link
Contributor

I meant to say that it'd be a shame if the React team doesn't want to make the move to 5.0.4 [because it's alpha], as NOT having 5.0.4 breaks stuff for a lot of consumers [of react-native powered apps].

Can you clarify on "breaks stuff for a lof of consumer"? Is there anything beyond Happy Eyeball? Also I invite you to raise this issue upstream on OkHTTP to promote the latest release to stable if it's so crucial.

Also, my question still holds:

What is preventing a consumer to bump OkHTTP version to whatever they wish?

Users should be able to update OkHTTP version regardless of the one we ship inside RN. If this is not possible, that's something we can look into, as I mentioned in one of my preivous comments:

Potentially try to adapt our codebase to make sure that is OkHTTP 5x compatible. That means that you'll be able to just bump the OkHTTP version to 5.x without having to make any changes or fork RN to make it work.

@marcesengel
Copy link
Contributor Author

marcesengel commented Dec 1, 2022

Can you clarify on "breaks stuff for a lof of consumer"?

Yes. There are quite a few ISPs (at least in Germany) where IPv6 isn't working properly. If a customer uses this kind of network, they are waiting a very long time until all their IPv6 requests have timed out and only then they can get a server response (when the first IPv4 address is tried).

Users should be able to update OkHTTP version regardless of the one we ship inside RN. If this is not possible, that's something we can look into, as I mentioned in one of my preivous comments:

I didn't know that was possible without building RN yourself. In my opinion it'd still be very beneficial to have the issue described above fixed by default and not requiring to fiddle with RN for this. I understand however that company policy states not to use 3rd party libs while in alpha.

@cortinico
Copy link
Contributor

I didn't know that was possible without building RN yourself. In my opinion it'd still be very beneficial to have the issue described above fixed by default and not requiring to fiddle with RN for this. I understand however that company policy states not to use 3rd party libs while in alpha.

It's possible as you can just specify a dependency on your build.gradle and the higher version of OkHTTP will override the one provided by React Native

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android fetch hangs indefinitely with IPv6 hosts on some devices (Happy Eyeballs)
3 participants