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

Implement Happy Eyeballs connection RFC #718

Closed
wants to merge 2 commits into from

Conversation

mrkline
Copy link

@mrkline mrkline commented Feb 3, 2024

Races interleaved IPv4 and IPv6 connections to provide the fastest one in cases where certain addresses or address families might be blocked, broken, or slow. (See https://datatracker.ietf.org/doc/html/rfc8305)

ureq strives for simplicity, and avoids spawning threads where it can, but - like with SOCKS - there's no way around it here.
Some mini internal async executor (discussed in #535 (comment)) wouldn't help - connect() is a blocking syscall with no non-blocking alternative. (Big async runtimes like Tokio "solve" this problem by keeping a pool of OS threads around for just these sorts of blocking calls.) We could have some thread pool (a la rayon) to avoid spawning threads on each connection attempt, but spawning a few threads is a cheap operation compared to everything else going on here. (DNS resolution, handshaking across the Internet...)

Much of this implementation was inspired by attohttpc's:
https://github.com/sbstp/attohttpc/blob/master/src/happy.rs

Fixes #535

@algesten
Copy link
Owner

algesten commented Feb 4, 2024

Hi @mrkline, welcome to ureq!

This is exciting, and a nice tidy-up. I actually have no idea what ureq users in general think about spawning more threads. It might be we've been unnecessarily conservative. We might want to have another simple::connect which behaves like today and make it possible to chose behavior in the Agent. However if we do that, I don't mind defaulting to eyeballs::connect, since I think it's a better solution.

@mrkline
Copy link
Author

mrkline commented Feb 11, 2024

Thanks for the kind intro! I fixed the CI issue - cargo doc noticed an import bug that cargo check/build didn't seem to. Odd...

I'll defer to you - if you want the behavior to be selectable in Agent, let me know. Though Happy Eyeballs - by trying our options concurrently - does solve the problem of how to split the timeout/deadline among multiple addresses. (Other libraries like attohttpc and minreq spawn threads to solve this problem.)

If we time out while connecting, don't tell the user we timed out
reading a response.
@rpodgorny
Copy link

hello, what is the plan on merging this? thanks.

@algesten
Copy link
Owner

hello, what is the plan on merging this? thanks.

I don't want to merge this in ureq 2.x because the connection code already there is too complex.

The rewrite in ureq3 branch is almost ready. I'm planning on releasing a beta very soon.

The happy eyeball strategy will fit much better into ureq3 where the transport is completely plugin. It can even be released as a separate crate.

@algesten
Copy link
Owner

Closing since we are moving to 3.x.

@algesten algesten closed this Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Happy Eyeballs to workaround slow/broken IPv6 connections
3 participants