Skip to content

Conversation

@florimondmanca
Copy link
Contributor

Fixes #565

There's a subtle case regarding handling multi-errors (i.e. what happens if coroutine2 raises an exception while we're handling an exception raised by coroutine2), but I'd suggest handling it in a subsequent PR. I left a comment in the code regarding this.

@florimondmanca florimondmanca added the concurrency Issues related to concurrency and usage of async libraries label Dec 6, 2019
@florimondmanca florimondmanca requested a review from a team December 6, 2019 09:15
@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Dec 6, 2019

Nice one!

I think we should probably look at using anyio's TaskGroups for the asyncio implementation... https://anyio.readthedocs.io/en/latest/tasks.html - that ensures we've got a proper nursery like primitive there.

I essentially want to start only using TaskGroups for any asyncio branching, since its the right approach™️

We can also be stronger on our wording wrt. MultiError, it's not that "we don't support it yet" it's that we specifically only care about raising one exception type. It's enough for the API to expose that eg. "A read timeout occured" or "A write timeout occured". There's no need for us to support exposing the case that "A read timeout and a write timeout occured". either one of those is sufficient for the operation as a whole to be cancelled, and it's actually okay in this circumstance for us to just return either indeterminatly.

# behavior here, and need to arbitrarily decide which exception to raise.
# We may want to add an 'httpx.MultiError', manually add support
# for this situation in the asyncio backend, and re-raise
# an 'httpx.MultiError' from trio's here.
Copy link
Contributor

Choose a reason for hiding this comment

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

We can be more direct on our wording here. If MultiError occurs then we semantically only want either one of those two exceptions raised up to the user.

try:
await task
except asyncio.CancelledError:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

This'll look a bunch nicer with anyio. (Tho we should leave Trio's implementation alone, since they already have a sensible nursery primitive.)

Copy link
Contributor

@lovelydinosaur lovelydinosaur left a comment

Choose a reason for hiding this comment

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

Added two comments in-place to clarfiy exactly.

This is looking great! 😃

@florimondmanca
Copy link
Contributor Author

Are we okay adding a dependency on anyio for this? We don’t need it — correctly handling errors in the case of 2 coroutines is easy enough, and the current implementation yields the expected behavior.

@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Dec 6, 2019

Yeah, I'm actually pretty okay with it...

I'm strongly enough of the camp that asyncio circa Python 3.8 does not provide suitable primitives for async task branching, and that a reasonable policy at this point in time is to never use the low level APIs there.

There's been talk of introducing a TaskSupervisor class into Python, which might also have a backport-installable package, in which case we could switch to that. Until that point in time the only sensible nursery-like-primitive for asyncio is the anyio implementation.

If we stick with that then we know we're properly adhering to the structued concurrency constraints, even in our asyncio implementation.

Once we've got some specifc async docs back in again I'd also like to help us lead our users down the structured concurrency path, and have us recommend anyio's TaskGroups to end users for task-branching with asyncio.

@lovelydinosaur
Copy link
Contributor

If we think that's being overly zealous, then I guess we could stick with a plain asyncio implementation there, but I'd be more comfortable with us leaning on anyio's TaskGroups.

@lovelydinosaur
Copy link
Contributor

Let's leave it as an open option.

This is looking great as it stands so we'll get this in, the later tweak the comment note on MutliError, and consider anyio. (It could also be that we don't strictly need it ourselves, but could highly recommend TaskGroups to end users)

@lovelydinosaur lovelydinosaur merged commit c38fd68 into master Dec 6, 2019
@lovelydinosaur lovelydinosaur deleted the refactor/background2fork branch December 6, 2019 10:49
@lovelydinosaur
Copy link
Contributor

👍✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

concurrency Issues related to concurrency and usage of async libraries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drop background manager (in favour of a two-function fork primitive).

3 participants