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

Implemented the Happy Eyeballs (RFC) algorithm for connect_tcp() #78

Merged
merged 8 commits into from
Oct 5, 2019

Conversation

agronholm
Copy link
Owner

No description provided.

This was done in preparation for the the happy eyeballs work on connect_tcp().
@agronholm agronholm requested review from smurfix and njsmith October 4, 2019 12:33
@agronholm
Copy link
Owner Author

I didn't look at the trio implementation until at the end. This differs from trio's in that it preemptively launches all the tasks at once. Not sure if there's any practical difference.

@agronholm
Copy link
Owner Author

agronholm commented Oct 4, 2019

Bizarrely all the Linux builds fail with trio as the backend on the test_happy_eyeballs_connrefused test. That doesn't happen locally for me. I'm not sure what's going on.

anyio/__init__.py Outdated Show resolved Hide resolved
anyio/__init__.py Outdated Show resolved Hide resolved
Trio's MultiError has a trick that returns a single exception as-is instead of wrapping it in a MultiError instance.
We do something similar here to produce expected results on all backends.
This change also saves us from spawning too many tasks unnecessarily.
Copy link
Collaborator

@smurfix smurfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC 8305 §3 talks about doing IPv4 and IPv6 lookups in parallel, i.e. using two separate queries. You should probably do that.

anyio/__init__.py Outdated Show resolved Hide resolved
@njsmith
Copy link
Collaborator

njsmith commented Oct 4, 2019

System administrators have a configuration knob they can set to control whether a host should prefer IPv4 or IPv6. The only portable way to figure out how this knob is set is to look at the sort order of addresses returned from getaddrinfo.

So Trio doesn't do IPv6 first and IPv4 second – it always keeps the first entry in getaddrinfo's return value in the first spot, and then makes sure the second entry uses a different family. That way we respect the system administrator's decision.

This is also why Trio just makes one call to getaddrinfo, instead of trying to race the IPv4 and IPv6 lookups against each other (like Apple's new RFC recommends). We could race them, but then we wouldn't know which one to prefer. Apple's version of happy eyeballs has privileged access to the OS and can see system configuration, correlate information across different processes, etc. Mere userspace libraries like ours don't have that option.

@agronholm
Copy link
Owner Author

I suspected this was the case. I will not try to race the DNS queries then.

@njsmith
Copy link
Collaborator

njsmith commented Oct 4, 2019

I guess I'll also raise a larger question. Should anyio include its own Happy Eyeballs implementation?

Context:

  • Trio comes with a Happy Eyeballs implementation
  • In Python 3.8+, asyncio will come with a Happy Eyeballs implementation: python/cpython@88f07a8
  • anyio may need to move away from implementing its own networking stack in order to keep supporting asyncio on Windows (Support the asyncio ProactorEventLoop #77), and this might also help anyio-based libraries feel "native" in different environments
  • So maybe anyio should use the underlying library's "open a TCP connection" functionality instead of implementing its own?

I guess an underlying tension is to what extent anyio wants to be a compatibility layer to make it easy to use multiple I/O backends, versus a full-featured networking library in its own right. I don't know the answer and this is very much a judgement call for @agronholm to make, but it seemed worth at least bringing it up explicitly.

@agronholm
Copy link
Owner Author

The reason I decided to make my own network stack in AnyIO is that the network semantics in the three supports async libraries were so wildly different that I felt it was too risky to rely on the native (high level) functionality.

In Python 3.8+, asyncio will come with a Happy Eyeballs implementation: python/cpython@88f07a8

Curio does not have a Happy Eyeballs implementation, nor do Python 3.7 and earlier. So giving this up is not really an option in the very near future.

So maybe anyio should use the underlying library's "open a TCP connection" functionality instead of implementing its own?

I'm still in the process of writing POC code using asyncio's low level functions, combined with memory BIO based TLS (just like trio does). If that pans out, I will not have to rewrite the entire network stack in AnyIO.

I guess an underlying tension is to what extent anyio wants to be a compatibility layer to make it easy to use multiple I/O backends, versus a full-featured networking library in its own right.

I intended AnyIO to be a compatibility layer. But to provide a uniform experience across all three libraries, I felt that I had to provide my own networking implementation. If I could use the high level native networking functionality of all three libraries without breaking compatibility, I'd be willing to cast out my own networking code in favor of wrappers.

@njsmith
Copy link
Collaborator

njsmith commented Oct 5, 2019

The reason I decided to make my own network stack in AnyIO is that the network semantics in the three supports async libraries were so wildly different that I felt it was too risky to rely on the native (high level) functionality.

Yeah... Hopefully the work on standard stream abstractions will help here. Which reminds me, I need to get back to that :-).

memory BIO based TLS (just like trio does).

One question I want to look at more closely after the streams stuff settles down is whether it would make sense to extract Trio's TLS code into a generic stream-helpers library. It's not quite implementable using only the stream APIs, but it's very close, so maybe?

Curio does not have a Happy Eyeballs implementation, nor do Python 3.7 and earlier. So giving this up is not really an option in the very near future. [...] to provide a uniform experience across all three libraries, I felt that I had to provide my own networking implementation

I guess a question that will keep coming up in different forms is whether the goal for anyio is to be consistent across backends, or consistent with backends. E.g. all the backends do provide a native "open a TCP connection to a hostname, with some kind of handling for multiple IP addresses" operation, and someone could argue that getting that native behavior might actually be what users expect.

I'm not saying you should or shouldn't ship your own happy eyeballs here; I don't think this is a question that has a single answer. For example, I think it's great that anyio only exposes a nursery-style task spawning interface on all backends :-). I'm just thinking out loud, trying to wrap my head around the design tradeoffs here.

@agronholm
Copy link
Owner Author

One question I want to look at more closely after the streams stuff settles down is whether it would make sense to extract Trio's TLS code into a generic stream-helpers library. It's not quite implementable using only the stream APIs, but it's very close, so maybe?

I'll have a look at this once the proverbial dust settles.

I guess a question that will keep coming up in different forms is whether the goal for anyio is to be consistent across backends, or consistent with backends.

To put it simply, my goal with AnyIO is to offer a trio-like API and to be consistent about it across backends. That is the only way I see it could work since the various backends have wildly differing ideas on how things like concurrency, networking etc. should work. The idea is that, since some features (like KeyboardInterrupt handling) cannot (I think) be offered by AnyIO, users could write their code against AnyIO and then switch to trio (or an evolved version of asyncio) as the backend. Without a bridge like AnyIO, the jump would be too intimidating. At least it would be for me.

@agronholm
Copy link
Owner Author

@sethmlarson Any further comments on the code now that I've fixed the flaws you pointed out? I'd like to get this merged.

Copy link

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@agronholm agronholm merged commit d924094 into master Oct 5, 2019
@agronholm
Copy link
Owner Author

Thanks to all reviewers!

@agronholm agronholm deleted the happyeyeballs branch October 5, 2019 12:48
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.

4 participants