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

Remove usage of loop.create_task in ASGIDispatch #261

Merged
merged 3 commits into from
Aug 21, 2019

Conversation

florimondmanca
Copy link
Member

Refs #247

Getting close 🎉

@tomchristie
Copy link
Member

I think everything here is in the right direction, yup.

We've obvs. got a weak point wrt. the background manager, as it's a shame that we're using it in a non-context managed way in the ASGI context ATM, but that's already the case with the existing implementation prior to this pull request anyways.

@tomchristie
Copy link
Member

The other relevant thing to highlight related to this is that we could still forego the background manager entirely if we switched to a "read and write for a bit" method on the Stream class.

That would mean a bit of work on the h11 and h2 implementations, but an advantage there is that the only bit of codebase that's then actually doing more than one task concurrently is kept strictly in the ConcurrencyBackend layer.

(We'd also need to think about if and how to restructure the ASGI implementation to properly take advantage of that)

@florimondmanca
Copy link
Member Author

Yeah, nice one on highlighting the weakness of the current usage of a background manager in the ASGI dispatcher @tomchristie.

I actually reluctant to write that manual call to .aexit() — and don’t think I did it before ever — so that’s a strong hint there’s room for thought for refactoring this bit, probably along with the refactoring of h11/h2 dispatchers as well, as you mentioned.

I guess it’s worth adding a comment to the code in ASGIDispatch about this? Can’t touch the code until tomorrow so feel free to go ahead and then merge once you feel it’s right. :)

@tomchristie
Copy link
Member

tomchristie commented Aug 21, 2019

I guess it’s worth adding a comment to the code in ASGIDispatch about this?

Yes I think so. A comment there would be reasonable.

Something else that we could consider if we really needed to would be making the ASGI dispatcher be asyncio only (at least until we'd figured out a refactoring) if we wanted to otherwise be able to switch to a read-and-write-for-a-bit method instead of a background manager.

I guess we probably wouldn't go down that route, but I think we can keep it open, just in case we're otherwise running into hard blockers.

@florimondmanca
Copy link
Member Author

From having tried adding complete trio support before in #214, I know the current background manager is enough to get something functional — there’s no particular blocker towards a trio backend here from my perspective.

@tomchristie
Copy link
Member

Happy to have this in when you are. Probably not a bad idea to comment and link to this issue, yeah.

@florimondmanca florimondmanca merged commit 80eafe8 into master Aug 21, 2019
@sethmlarson sethmlarson deleted the refactor/asgi-background branch August 28, 2019 00:14
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.

2 participants