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

Switching to anyio as the default backend when running under asyncio. #344

Closed
tomchristie opened this issue May 24, 2021 · 13 comments · Fixed by #345
Closed

Switching to anyio as the default backend when running under asyncio. #344

tomchristie opened this issue May 24, 2021 · 13 comments · Fixed by #345

Comments

@tomchristie
Copy link
Member

tomchristie commented May 24, 2021

Having been bitten by some rough edges in asyncio's SSL support, and having taken a look over anyio's TLSStream implementation. (Which just plain makes sense *) I'm now very warm on the idea of us switching over to anyio as the default backend for the asyncio case.

We could consider deprecating and later removing the native curio.py and asyncio.py modules, but we don't necessarily need to do that to start with.

First step here would be switching the default, and issuing a release, in order to make sure we hit any unexpected issues that might crop up as a result.

Paging @agronholm. 😀


*: In contrast to this, what the heck is loop.start_tls? Why is that a property of the event loop? Etc.

@agronholm
Copy link
Contributor

Hooray! 💯

As for curio support, you would have to keep that in anyways since AnyIO has dropped the curio backend (at the request of curio's author).

Is there anything you need from me?

@tomchristie
Copy link
Member Author

since AnyIO has dropped the curio backend (at the request of curio's author)

Okay. Just for context, is that a private request, or on a public thread?

Is there anything you need from me?

Up to you. I'm happy to put in the PR myself, since I've got paid OSS time which I can use to do it, but equally if you were keen to get it in because it's your baby, then you're also welcome to take it on yourself. 🤷‍♂️😁

Aside: It's interesting to me that Trio's TLSStream implementation has so much more complex locking than the anyio case. I started to have a bit of a deeper dig into that, but just mentioning it here for now.

@agronholm
Copy link
Contributor

Okay. Just for context, is that a private request, or on a public thread?

agronholm/anyio#185

Aside: It's interesting to me that Trio's TLSStream implementation has so much more complex locking than the anyio case. I started to have a bit of a deeper dig into that, but just mentioning it here for now.

I looked at the trio implementation before I made my own; I just couldn't figure out why it was so damn complex.

@agronholm
Copy link
Contributor

I'll try to get a PR done this week.

@tomchristie
Copy link
Member Author

tomchristie commented May 24, 2021

Noting it here while I remember - we almost certainly want to set Trio's https_compatible=True and AnyIO's standard_compatible=False to the SSLStream constructors.

Also AsyncHTTPProxy is not currently passing though the backend argument to AsyncHTTPConnection instances, so setting eg. AsyncHTTPProxy(..., backend="anyio|auto|asyncio") does not currently work as expected. Nobody has bumped into that because, well, everyone's just using "auto", because of course they are.

@agronholm
Copy link
Contributor

Noting it here while I remember - we almost certainly want to set Trio's https_compatible=True and AnyIO's standard_compatible=False to the SSLStream constructors.

The AnyIO backend has done this from the start IIRC, but the trio backend currently doesn't.

@agronholm
Copy link
Contributor

One question remains – is it okay to require the latest anyio as a hard dependency? if so, I'll remove the compatibility code from the backend then.

@agronholm
Copy link
Contributor

One other thing I noticed – all the async tests seem to be running only on trio, or am I missing something?

@agronholm
Copy link
Contributor

Just most of them actually – they're marked with @pytest.mark.trio while others are marked @pytest.mark.anyio.

@agronholm
Copy link
Contributor

Is there any reason at all to use pytest-trio here?

@tomchristie
Copy link
Member Author

One question remains – is it okay to require the latest anyio as a hard dependency? if so, I'll remove the compatibility code from the backend then.

Yes. Tho question - are there any API differences between 3.0 and 3.1 that impact us? If we can depend on 3.x then let's do that by preference, otherwise, sure 3.1+.

Is there any reason at all to use pytest-trio here?

Not sure without digging, but let's not change anything there as part of any initial PR here.

@agronholm
Copy link
Contributor

AnyIO uses semantic versioning so v3.1 is API compatible with v3.0. It's that httpcore's AnyIO backend was made for AnyIO v2.0 originally, and if anybody requires the older major release then this bump might negatively affect them. My other main concern was adding AnyIO as a hard dependency, regardless of version.

Not sure without digging, but let's not change anything there as part of any initial PR here.

If indeed most tests are only run on trio and not the other backends then this needs to be fixed ASAP, even if in a separate PR.

@tomchristie
Copy link
Member Author

If indeed most tests are only run on trio and not the other backends then this needs to be fixed ASAP, even if in a separate PR.

Well, yes and no. We don't always need to exercise down to "repeat this test with each individual backend". Eg. if we're testing some connection pooling behaviour, or some part of HTTP/2 fiddlieness, or suchlike, then it's not a terrible thing to just test that against one given backend, so long as we do also have sufficient coverage over testing each of the backends in general. So, mixed bag, but in either case let's treat any convo there as a separate thing.

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 a pull request may close this issue.

2 participants