Fix compatibility with third-party sync-only middleware

[Archmonger]

Description

• Fix GCP Middleware fails to run with ServeStatic #18
• Disable Django sync middleware capability to avoid issues with Django's usage of asgiref.AsyncToSync

Checklist

Please update this checklist as you complete each item:

• Tests have been developed for bug fixes or new functionality.
• The changelog has been updated, if necessary.
• Documentation has been updated, if necessary.
• GitHub Issues closed by this PR have been linked.

_{By submitting this pull request I agree that all contributions comply with this project's open source license(s).}

[Archmonger]

@brianglass can you see if your GCP middleware works with this branch?

pip install git+https://github.com/Archmonger/ServeStatic@fix-gcp-middleware

[brianglass]

@Archmonger, yes, that works.

[brianglass]

@Archmonger, I deployed this to production and it is working well. I can confirm that the warnings have gone away.

[Archmonger]

Fairly frustrating since this proves how incredibly brittle async support is within Django.

I'll work on getting this merged soon.

[brianglass]

Yes, it's pretty difficult to get your stack clear of synchronous middleware. I should probably look for a way to get rid of the Google middleware since it causes a context switch. it's frustrating to go through all the effort of building everything async only to need something like this in the end.

[brianglass]

For future reference to anyone reading this conversation, I created a wrapper around the Google logging middleware to make it async compatible. The middleware doesn't actually do anything other than assign the request object to a thread local.

from asgiref.sync import iscoroutinefunction
from django.utils.decorators import sync_and_async_middleware
from google.cloud.logging_v2.handlers.middleware import RequestMiddleware

@sync_and_async_middleware
def google_logging_middleware(get_response):
    if iscoroutinefunction(get_response):
        async def middleware(request):
            response = await get_response(request)
            google_middleware = RequestMiddleware(lambda request: response)
            return google_middleware(request)
    else:
        middleware = RequestMiddleware(get_response)

    return middleware

[Archmonger]

@brianglass I just pushed a different attempt at fixing this issue. Let me know if it works in your environment.

[brianglass]

@Archmonger, that again produces the following error:

web-1  | ERROR Internal Server Error: /readings/gregorian/2024/8/23/
web-1  | Traceback (most recent call last):
web-1  |   File "/usr/local/lib/python3.12/site-packages/django/core/handlers/exception.py", line 55, in inner
web-1  |     response = get_response(request)
web-1  |                ^^^^^^^^^^^^^^^^^^^^^
web-1  |   File "/usr/local/lib/python3.12/site-packages/google/cloud/logging_v2/handlers/middleware/request.py", line 48, in middleware
web-1  |     return get_response(request)
web-1  |            ^^^^^^^^^^^^^^^^^^^^^
web-1  |   File "/usr/local/lib/python3.12/site-packages/asgiref/sync.py", line 187, in __call__
web-1  |     raise RuntimeError(
web-1  | RuntimeError: You cannot use AsyncToSync in the same thread as an async event loop - just await the async function directly.

If I switch to using the wrapped middleware I posted above, the error goes away.

[Archmonger]

Okay - I've reverted back to the solution that initially worked. Unfortunately Django is extremely sensitive to mixed sync/async contexts, so unless some changes happen to asgiref/django our middleware will now be purely async.

The downside of this change is that Django WSGI will now need to convert our middleware from async to sync.

On the bright side, if we can keep things purely async it makes maintainability a lot easier!

If this becomes a problem for WSGI users, I will need to revert this PR and kludge together my own variant of asgiref.async_to_sync that always runs in a temporary thread. Would be a similar solution to the servestatic.middleware.AsyncToSyncIterator utility function I developed.
EDIT: I tested this and it was not a solution unfortunately.

[brianglass]

Now that I've made such a todo about this, I realize that the Google logging middleware may not even be safe for use in async applications. Since it is using a thread-local to share the request object with the logger and async Django might be processing multiple requests concurrently in a single thread, it doesn't make sense to use this. I'll be looking for an alternative or simply going back to vanilla logging.

[Archmonger]

Django does not reuse threads. It is one thread per request.

[brianglass]

According to the Django async Docs, "The main benefits are the ability to service hundreds of connections without using Python threads. This allows you to use slow streaming, long-polling, and other exciting response types." I take that to mean that multiple requests can be handled in a single thread. So instead of using a thread local, one should use contextvars to manage state. These docs say, "Context managers that have state should use Context Variables instead of threading.local() to prevent their state from bleeding to other code unexpectedly, when used in concurrent code."

Am I missing something?

[Archmonger]

The explanation is a bit of a sore point for me, since it drives home how async is still half-baked in Django.

Upon the release of Django 3.1, the asgiref converter functions (sync_to _async and async_to_sync) ran within a thread pool. I believe it was something like CPU_CORES * 2 amount of threads by default. Unfortunately, the Django team quickly realized that this was causing silent corruption of the ORM. The Django ORM is lazy, so it starts a DB connection and only finishes execution when when a value is needed, which was problematic for some of the DB backends. They did not want to rewrite any aspects of the ORM since that might impact sync performance, especially given that there was a decision made that Django would prioritize sync at all costs. So instead, they forced all of those converter functions to run on the main thread. This effectively meant that async code was run "purely sync" except with even worse performance due to stack complexity.

Enough people complained how cripplingly bad Django's async performance was, so around the release of Django ~4.0, they opted for a "middle ground" solution, where Django would create a temporary thread for each HTTP request and use that as a safe execution environment when needed whenever conversion is needed within the Django stack.

There's still always an async performance penalty under the hood since Django utilizes this converter function all over their source code, but at least it's not absolutely crippling now.

However, to this day all ORM, cache, and template rendering is still performed directly on the main thread via async_to_sync(thread_sensitive=True), which stalls the ASGI queue and forces a context-switch performance hit.

Things are more performant if you step outside the Django stack (such as the way channels sits on-top of Django's HTTP stack). This unfortunately means you only get the most benefits of ASGI if you avoid entering Django entirely.

I haven't had time to benchmark ServeStatic, but I suspect this also means that using servestatic.middleware.ServeStaticMiddleware is likely significantly less performant than wrapping the entire Django application in servestatic.ServeStaticASGI.

[brianglass]

Thanks for that detailed explanation @Archmonger.