search

fanout / django-eventstream

Server-Sent Events for Django
MIT License
664 stars 88 forks source link

test_stream.py error with Channels 4 #100

Closed paolodina closed 7 months ago

paolodina commented 1 year ago 
$ python tests/test_stream.py 
Traceback (most recent call last):
  File "/tmp/django-eventstream/tests/test_stream.py", line 6, in <module>
    from django_eventstream.consumers import EventsConsumer, Listener
  File "/tmp/django-eventstream/.venv/lib/python3.10/site-packages/django_eventstream/__init__.py", line 17, in <module>
    from . import routing
  File "/tmp/django-eventstream/.venv/lib/python3.10/site-packages/django_eventstream/routing.py", line 2, in <module>
    from . import consumers
  File "/tmp/django-eventstream/.venv/lib/python3.10/site-packages/django_eventstream/consumers.py", line 7, in <module>
    from channels.http import AsgiRequest
ModuleNotFoundError: No module named 'channels.http'

Everything fine downgrading Channels to v3.0.5.

https://channels.readthedocs.io/en/latest/releases/4.0.0.html

jkarneges commented 1 year ago

It looks like this has to do with the removal of AsgiRequest which happened here.

Django doesn't offer the same class so I'm not sure what we're supposed to switch to. Maybe the standard request/response objects. I've asked the maintainer.

If it turns out we should switch to standard Django objects, then that'd be pretty exciting since we've been waiting years to be able to do that. But porting the code won't be simple, so users should stick to channels 3 until this can be investigated.

paolodina commented 1 year ago

Thanks for the clear answer. Sounds intriguing, please keep us informed.

jpmateo022 commented 1 year ago

It looks like this has to do with the removal of AsgiRequest which happened here.

Django doesn't offer the same class so I'm not sure what we're supposed to switch to. Maybe the standard request/response objects. I've asked the maintainer.

If it turns out we should switch to standard Django objects, then that'd be pretty exciting since we've been waiting years to be able to do that. But porting the code won't be simple, so users should stick to channels 3 until this can be investigated.

I reverted to 3.0.5 to make it work. Will wait for the new version.

markandrewj commented 1 year ago

I encountered this same issue using channels 4 with Django 4. It seems like a possible solution is to remove channels all together and use Pushpin in front of the Django according to the readme.

It is possible to use this library with a GRIP proxy only, without setting up Channels.

Does using channels provide anything besides ease of development? I was thinking about trying to add Pushpin into my project as a container. I know this was brought up in another issue, but it would be helpful to have a helm chart or an operator packaged for Pushpin as well.

jkarneges commented 1 year ago

Does using channels provide anything besides ease of development?

Nope. If you're planning to use Pushpin in production, then all channels does is enable the use of runserver without Pushpin during development.

gagamil commented 1 year ago

Ran into the same issue. The details are here.

My solution was to use the previous channels version which is 3.0.5

dpgraham4401 commented 1 year ago

It looks like it's possible to replace ASGIRequest from channels.http with django.core.handlers.asgi.ASGIRequest i was able to successfully implement in a POC branch here https://github.com/dpgraham4401/django-eventstream/tree/channels_4_support_POC.

image

Heads up @jkarneges

ellethee commented 1 year ago

It looks like it's possible to replace ASGIRequest from channels.http with django.core.handlers.asgi.ASGIRequest

I adopted the same solution and it seems to work.

jkarneges commented 1 year ago

It looks like it's possible to replace AsgiRequest from channels.http with django.core.handlers.asgi.ASGIRequest

[edited case in quote]

Great find @dpgraham4401! It appears ASGIRequest has been available in Django for a while, forked from the original in Channels. Maybe I missed it due to the case change.

Simply switching to the new class doesn't work for me though, at least with django-eventstream's chat demo. Connections are immediately disconnected and the browser reconnects over and over. I'll do more investigation.

jkarneges commented 7 months ago

Ok! I've ported the code to use StreamingHttpResponse instead of channels. Apparently it has been async capable since Django 4.2. However it wasn't until 5.0 that it could handle disconnections properly.

Since it's a pretty big change, can anyone else try it out before I publish a new version?

paolodina commented 7 months ago

Hi @jkarneges, unfortunately, I don't have access to the application to test it with this fix. FWIW, I tested the examples in a codespace and they worked great. I also had a look at the diff, and in my limited experience, it looks good. Thanks for taking the time to work on this. I'll leave it to you to close the issue.

jkarneges commented 7 months ago

Published! No more channels compatibility issue as channels is no longer needed.