Skip to content

[Android] Added Happy Eyeballs for address selection - #33045

Closed
marcesengel wants to merge 1 commit into
react:mainfrom
marcesengel:fix/android-happy-eyeballs-main
Closed

[Android] Added Happy Eyeballs for address selection#33045
marcesengel wants to merge 1 commit into
react:mainfrom
marcesengel:fix/android-happy-eyeballs-main

Conversation

@marcesengel

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

@cortinico some interesting updates on the matter: lysine-dev/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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

marcesengel commented Dec 1, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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