Support ASGI on Django 4.2+ #148

Alexerson · 2023-02-01T23:31:34Z

Alexerson · 2023-02-01T23:36:38Z

After pushing this first draft, I’m noticing that this won’t work anymore with WSGI because StreamingHttpResponse will now expect a sync generator.
I’m not too sure how we can have a view that will work for sync and async. Is there a way to detect? Maybe from the request?
In any case, I’ll look into that again later this week (hopefully).

Also, mypy is not happy with what I did, but I’m not sure why that is (especially the func-returns-value one)

And finally, I should probably add some async tests.

Alexerson · 2023-02-02T00:36:26Z

Ok, so I read https://code.djangoproject.com/ticket/33735#comment:8 after my first comment. So we would definitively need to provide both sync and async generator and figure out a way to know in which case we are.

carltongibson

Take a look at the docs for StreamingHTTPResponse in Django 4.2. There is says that the iterator needs to match whether you're running under WSGI or ASGI.

We can't just make this an async function and iterator, because most folks at still running under WSGI, and it just won't work for earlier Django versions.

I think we're likely going to need two views, and to tell folks to route the appropriate one depending on how they're running Django. (I'm not sure we can detect that? 🤔)

Maybe there's a nicer solution... 🤔 -- but I'd get that running first.

adamchainz · 2023-02-02T07:07:02Z

We can detect ASGI with isinstance(request, ASGIRequest), no?

src/django_browser_reload/views.py

carltongibson · 2023-02-02T07:25:03Z

We can detect ASGI with isinstance(request, ASGIRequest), no?

That might work likely, yes. 🤔

The view doesn't necessarily have to be async itself. Just the iterator passed to the response.

Alexerson · 2023-02-02T15:33:22Z

Thanks.

My first hope is to be able to detect if it's an ASGI or WSGI context from the view. If this doesn't work, then 2 views, and have the Middleware (that should be aware?) insert the proper url?
Anyway, I'll get to it properly again later today.

Alexerson · 2023-02-14T23:31:55Z

Ok, so I started from scratch and went for something simpler.
It seems to work when I run it on a side project, but the tests are failing. For some reason Django is not detecting that my generator is async, and I end up with a map in my tests. I’ll take a bit more time on this tomorrow.

Also, I kind of randomly picked HTTPStatus.EXPECTATION_FAILED for the response, but it might not be the most helpful and we should definitively come up with something easier to understand for the end user.

Alexerson · 2023-02-14T23:40:15Z

Something that we will probably want to look at as well: I get the following messages when testing it:
WARNING 2023-02-14 18:38:16,543 server 1153246 140605946582720 Application instance <Task pending name='Task-1' coro=<ASGIStaticFilesHandler.__call__() running at /app/.venv/lib/python3.11/site-packages/django/contrib/staticfiles/handlers.py:101> wait_for=<Future pending cb=[Task.task_wakeup()]>> for connection <WebRequest at 0x7fe1635f56d0 method=GET uri=/__reload__/events/ clientproto=HTTP/1.1> took too long to shut down and was killed.

Alexerson · 2023-02-15T18:06:12Z

I looked a bit more today and tracked the issue to the fact that we are going through https://github.com/django/django/blob/6bdc3c58b65eb32fd63cd41849f00a17a36b4473/django/test/client.py#L110
in the async client. I don’t have more time this morning but will try to get back to it tonight.

I’ll actually look at bit more in how the async tests are written in the framework, because it feels weird that we can just do

response.streaming_content = await sync_to_async(
                closing_iterator_wrapper, thread_sensitive=False
            )(
                response.streaming_content,
                response.close,
            )

with an async generator and expect this to work 🤔

Alexerson · 2023-02-16T00:02:37Z

I’m now convinced there is a bug in how django’s async_client behave.
See https://code.djangoproject.com/ticket/34342

Alexerson · 2023-02-21T17:33:05Z

The bug is now fixed in the beta version of 4.2, so I think except the decision that needs to be taken for how to tackle the error for async and Django < 4.2, this is now ready for a proper review.
(the mypy error are to be fixed upstream as well I believe)

Alexerson · 2023-03-20T15:03:28Z

Hey,

With Django 4.2 coming up officially in a couple weeks, it would be great to have this merged.

Is there anything else I can do to get this moving?

abdulrabbani00 · 2023-04-24T00:11:46Z

If this works, can we get it merged. I would love to use tailwind but cant since it keep crashing.

Alexerson · 2023-05-09T22:17:05Z

I took some time to cleanup my branch and sync it with main. I also changed the behaviour with Django < 4.2.

All checks are now passing (https://github.com/Alexerson/django-browser-reload/pull/1/checks), but it seems the pre-commit hooks are still failing…

I’m opening typeddjango/django-stubs#1483 to have it fixed upstream.

Alexerson · 2023-05-26T19:43:14Z

Now that typeddjango/django-stubs#1484 is merged, I ran mypy locally again and found some issues, all related to the fact that streaming_content in an iterable and not an iterator.
This is related to the comment I got here: django/django#16559 (review)

I think it’s just a matter of doing:

-event = next(response.streaming_content)
+response_iterable = iter(response)
+event = next(response_iterable)

in a few places. I feel this should be a different PR though (probably the one in which you’ll move to django-stubs==4.2.1 once released) that we will have to wait for before being able to merge this.

Edit: The whole changes would be something like this (obviously, the version is not released yet, so this would fail): Alexerson@c58f57f

adamchainz · 2023-06-05T10:09:34Z

Thank you for keeping this updated all this time! I am looking into it now.

Alexerson · 2023-06-05T15:41:03Z

Of course! This forced me to finally commit something in django/django and look deeper into django-stubs so I’m glad I persisted.

adamchainz · 2023-06-05T18:10:03Z

src/django_browser_reload/views.py

+        if DJANGO_VERSION < (4, 2):
+            err_msg = "We cannot support hot reload with ASGI for Django < 4.2"
+            warnings.warn(err_msg, stacklevel=2)
+            return HttpResponse(status=HTTPStatus.NOT_ACCEPTABLE)


I think the JavaScript should handle this and log a message as the connection fails.

adamchainz

Working on these changes.

adamchainz · 2023-06-05T18:10:32Z

src/django_browser_reload/views.py

+        response = StreamingHttpResponse(
+            event_stream(),
+            content_type="text/event-stream",
+        )


can be extracted to common branch by using event_stream as the name for the async generator too

adamchainz · 2023-06-05T18:11:17Z

src/django_browser_reload/views.py

+            err_msg = "We cannot support hot reload with ASGI for Django < 4.2"
+            warnings.warn(err_msg, stacklevel=2)


not sure a Python warning is helpful, leaving a message in the browser console seems more logical

After some playing around, leaving a browser console message would be unnecessarily complicated. I'm only going to put it in the HTTP response content, which users can debug if necessary. The ASGI support is documented as Django 4.2+ so hopefully no one will be surprised when it doesn't work on older versions.

Sounds good, especially because this was not working at all up until now (and was actually hanging the browser), so it’s not breaking any existing support, and anyone wanting to use it will (hopefully) read the README and/or changelog.

adamchainz · 2023-06-05T21:01:10Z

example/README.rst

+Run the sync WSGI server with:
+
+.. code-block:: sh
+
+   DEBUG=1 python manage.py runserver --noasgi
+
+Run the async ASGI server with:
+
+.. code-block:: sh


adding ASGI to the example project was key to testing this... did you not see the example project before? You tested only in your own project?

If I remember correctly, it was just easier for me to simply add this to my own project than to run the example project. But as this was a couple months ago, my memory might fail me and it’s also very well possible that I just didn’t see it.

adamchainz · 2023-06-05T21:01:59Z

tests/test_views.py

+        if sys.version_info >= (3, 10):
+            event = await anext(aiter(response))
+        else:
+            event = await response.__aiter__().__anext__()


repeating this pattern is a bit verbose... adding backports of anext / aiter

adamchainz · 2023-06-06T08:05:57Z

src/django_browser_reload/views.py

+                await asyncio.sleep(PING_DELAY)
+                yield message("ping", versionId=version_id)
+
+                if should_reload_event.is_set():
+                    should_reload_event.clear()
+                    yield message("reload")


I just realized that this is not using an asyncio event, but always sleeping a second between checks. That's imposing an average of 0.5 seconds delay on reloads, which is small but noticeable. Fixing in #178.

carltongibson reviewed Feb 2, 2023

View reviewed changes

adamchainz reviewed Feb 2, 2023

View reviewed changes

src/django_browser_reload/views.py Outdated Show resolved Hide resolved

Alexerson force-pushed the asgi branch 2 times, most recently from 4bfdee4 to c3a178d Compare February 14, 2023 23:29

Alexerson marked this pull request as ready for review February 21, 2023 17:33

Alexerson force-pushed the asgi branch 2 times, most recently from e9e08fc to da67324 Compare May 9, 2023 22:14

Alexerson and others added 3 commits June 5, 2023 11:24

Add support for ASGI

db38a0d

Fix stacklevel error

b36ce30

Changelog note

bbc5693

adamchainz force-pushed the asgi branch from 33c2f06 to bbc5693 Compare June 5, 2023 10:25

Upgrade example app requirements

5b6b446

Add ASGI mode to example app

049cdbc

adamchainz reviewed Jun 5, 2023

View reviewed changes

adamchainz requested changes Jun 5, 2023

View reviewed changes

adamchainz added 5 commits June 5, 2023 20:59

Extract event_stream definition

57a1709

README note WSGI/ASGI support

3233cad

Note required version

4b562e2

Only inject tag on ASGI on Django 4.2+

36dbe4b

Actually continue to return an error response

ce4de8f

adamchainz changed the title ~~Add support for ASGI (#87)~~ Support ASGI on Django 4.2+ Jun 5, 2023

adamchainz reviewed Jun 5, 2023

View reviewed changes

Backport aiter / anext

5b8034b

adamchainz enabled auto-merge (squash) June 5, 2023 21:07

adamchainz merged commit d697421 into adamchainz:main Jun 5, 2023

This was referenced Jun 5, 2023

Doesn't work with ASGI #87

Closed

Remove delay from ASGI reloading #178

Closed

adamchainz reviewed Jun 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ASGI on Django 4.2+ #148

Support ASGI on Django 4.2+ #148

Alexerson commented Feb 1, 2023 •

edited

Loading

Alexerson commented Feb 1, 2023 •

edited

Loading

Alexerson commented Feb 2, 2023

carltongibson left a comment

adamchainz commented Feb 2, 2023

carltongibson commented Feb 2, 2023 •

edited

Loading

Alexerson commented Feb 2, 2023

Alexerson commented Feb 14, 2023 •

edited

Loading

Alexerson commented Feb 14, 2023 •

edited

Loading

Alexerson commented Feb 15, 2023

Alexerson commented Feb 16, 2023

Alexerson commented Feb 21, 2023 •

edited

Loading

Alexerson commented Mar 20, 2023

abdulrabbani00 commented Apr 24, 2023

Alexerson commented May 9, 2023 •

edited

Loading

Alexerson commented May 26, 2023 •

edited

Loading

adamchainz commented Jun 5, 2023

Alexerson commented Jun 5, 2023

adamchainz Jun 5, 2023

adamchainz left a comment

adamchainz Jun 5, 2023

adamchainz Jun 5, 2023

adamchainz Jun 5, 2023

Alexerson Jun 5, 2023

adamchainz Jun 5, 2023

Alexerson Jun 5, 2023 •

edited

Loading

adamchainz Jun 5, 2023

adamchainz Jun 6, 2023

		err_msg = "We cannot support hot reload with ASGI for Django < 4.2"
		warnings.warn(err_msg, stacklevel=2)

Support ASGI on Django 4.2+ #148

Support ASGI on Django 4.2+ #148

Conversation

Alexerson commented Feb 1, 2023 • edited Loading

Alexerson commented Feb 1, 2023 • edited Loading

Alexerson commented Feb 2, 2023

carltongibson left a comment

Choose a reason for hiding this comment

adamchainz commented Feb 2, 2023

carltongibson commented Feb 2, 2023 • edited Loading

Alexerson commented Feb 2, 2023

Alexerson commented Feb 14, 2023 • edited Loading

Alexerson commented Feb 14, 2023 • edited Loading

Alexerson commented Feb 15, 2023

Alexerson commented Feb 16, 2023

Alexerson commented Feb 21, 2023 • edited Loading

Alexerson commented Mar 20, 2023

abdulrabbani00 commented Apr 24, 2023

Alexerson commented May 9, 2023 • edited Loading

Alexerson commented May 26, 2023 • edited Loading

adamchainz commented Jun 5, 2023

Alexerson commented Jun 5, 2023

Choose a reason for hiding this comment

adamchainz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexerson Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexerson commented Feb 1, 2023 •

edited

Loading

Alexerson commented Feb 1, 2023 •

edited

Loading

carltongibson commented Feb 2, 2023 •

edited

Loading

Alexerson commented Feb 14, 2023 •

edited

Loading

Alexerson commented Feb 14, 2023 •

edited

Loading

Alexerson commented Feb 21, 2023 •

edited

Loading

Alexerson commented May 9, 2023 •

edited

Loading

Alexerson commented May 26, 2023 •

edited

Loading

Alexerson Jun 5, 2023 •

edited

Loading