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

Migrate routers to use ASGI v3 single callables #1421

Closed

Conversation

jaydenwindle
Copy link
Contributor

This PR fixes #1420 by modifying ProtocolTypeRouter, URLRouter and ChannelNameRouter to be single callable ASGI v3 applications. It also maintains backwards compatibility with existing Channels consumers and applications by wrapping each router's nested applications with asgiref.compatibility.guarantee_single_callable.

See parent issue: #1319

@sidharthramesh
Copy link

When will this be merged? I'm unable to use the latest version of channels because of the error. Is there any simple workaround?

@carltongibson
Copy link
Member

Hi @sidharthramesh. We'll look for a new version supporting ASGI v3 throughout in the next period. Before Django 3.1 is the obvious goal.

Personally, I have a commitment to DRF to fulfil and then Channels is top of my list. If you'd like to help the effort that would be great.

The current version still works but, if you're in a rush you can always deploy this branch directly. (Pip has very good VCS support.)

@sidharthramesh
Copy link

Thank you! I figured out what I needed. I was trying to use the Django ASGI object instead of the ProtocolTypeRouter provided by channels. Reading the latest version of the tutorials helped!

@carltongibson
Copy link
Member

I was trying to use the Django ASGI object...

Yes, so the goal here would be to make that work.

You'd wrap your Django v3(.1) app in ProtocolTypeRouter to have Channels handle websockets etc. (You might branch by URL to use Channel's HTTP consumers too, but it's the same idea.)

Currently you still need to use the Channels supplied handler to serve your Django app. We'll keep that around, since Django 2.2. has two years left to run, but we'll guide folks to the Django supplied option once they're on 3.0+.

@stephendwolff
Copy link

Thank you! I figured out what I needed. I was trying to use the Django ASGI object instead of the ProtocolTypeRouter provided by channels. Reading the latest version of the tutorials helped!

ie, use the channels ProtocolTypeRouter in routing.py:

ASGI_APPLICATION = "<project>.routing.application"

not Django's asgi handler in asgi.py:

ASGI_APPLICATION = "<project>.asgi.application"

until this is sorted out.

@jheld
Copy link
Contributor

jheld commented Jul 8, 2020

Will something like this be needed for django 3.1? Given the increased django/asgi/asynchrony support it yields.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hi @jaydenwindle.

Sorry for the massive delay in uptake here. 2020. (Hope that covers it. 🙂)

Do you still have capacity to work on this? If not no stress, I can pick it up, but if so, super. Let's get it in.

I think we need to jump in with both feet: I don't think wrapping every application in guarantee_single_callable is a great way forward.

Rather, we'll bump the major version and say to users that they should update their applications or use asgiref.compatibility.double_to_single_callable() themselves. That's one time at configuration for them vs overhead on every request otherwise. Make sense?

Other than that this looks great.

@carltongibson carltongibson self-assigned this Jul 15, 2020
@jheld
Copy link
Contributor

jheld commented Aug 1, 2020

@carltongibson are the changes for finalization fairly straightforward (appears to be from the comments)? I should have some time this week to fork this initial change and implement it on a new PR.

@carltongibson
Copy link
Member

Hi @jheld — No, there's not much at it: it looks great.

The issue is just™ that I think the migration needs to be done in one-step, with a major version bump, so there's this PR, #1412, and then the consumers to update too. (That last is not massive I think: adjust the base classes, fix the tests. 🤞)

The hold-up is just me having time to pull that together, and doc up the changes, but if you have a bit of bandwidth to join in that would be super.

My plan is to pull the changes into a single branch, make sure they're all ready and then merge back in. If you wanted to start by combining @jaydenwindle's two PRs into a single one and rebasing that would be great.

@carltongibson
Copy link
Member

Rebased in #1479. As I said there:

Merging to the asgi-3 branch, so we can get that done before merging back to master.

Thanks for the effort @jaydenwindle!

@carltongibson
Copy link
Member

carltongibson commented Aug 3, 2020

Hi @jheld — I've merged the two PRs into an new branch for ASGI 3 compat in #1480.

There's TODOs there if you do have capacity. 🙂 (Simplifying BaseMiddleware might be a nice one — it seems a single hook would be enough... — Just __call__() maybe? It's only the single middleware in the project that uses it.)

@jheld
Copy link
Contributor

jheld commented Aug 3, 2020

@carltongibson that's awesome! thank you for combining those. I'll fork and build on top of that branch. I'll try to communicate my progress and capacity. Exciting! And thank you for the suggestions.

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.

Add ASGI v3 support to Channels Routers
5 participants