Log sync/async adaptations in MiddlewareMixin.__acall__()

[Mykhailo Havelia]

Follow-up to #37177, for the part that was deferred there.

#37177 fixed per-hook context switching by setting async_capable = False on Django's built-in middleware. The cause is still in MiddlewareMixin: it declares async_capable = True by default, so any subclass with sync hooks goes through __acall__(), which wraps each hook in its own sync_to_async().

This defeats load_middleware(), which is designed to adapt once per contiguous block of sync middleware.

So every user and third-party middleware written the documented way (subclass MiddlewareMixin, implement sync process_request / process_response) still has the problem #37177 measured. A default project that adds one or two such middleware between the built-in ones also splits the sync block, adding more switches around them.

A side effect: these switches are invisible. __acall__() calls sync_to_async() itself, so adapt_method_mode() never logs "Asynchronous handler adapted for middleware ...", which is the tool the docs point users to for finding this.

Proposal

https://code.djangoproject.com/ticket/37177#comment:7

Deprecate the default MiddlewareMixin.__acall__() implementation: raise a deprecation warning when a subclass ends up using it with sync hooks (i.e. sets async_capable = True explicitly but doesn't override __acall__()). After the deprecation period, remove it and make async_capable = False the default.

[Sarah Boyce]

Thank you for the follow-up!

I'm not convinced that deprecating MiddlewareMixin.__acall__() is necessary, as the current behavior is a documented performance trade-off. Any change to this behavior would need a wider discussion. You can start a forum discussion to propose this idea and see if the community agrees with your proposal: ​https://forum.djangoproject.com/c/internals/async/8

However, I agree that improving the visibility of these context switches would be useful. Let's focus this ticket on adding logging for the sync_to_async() calls in MiddlewareMixin.__acall__(). Accepting on that basis.

If your deprecation proposal has community by in, you can later create another ticket linking the forum discussion.

[Mykhailo Havelia]

I'm not convinced that deprecating MiddlewareMixin.acall() is necessary, as the current behavior is a documented performance trade-off. Any change to this behavior would need a wider discussion.

I think "deprecation" was the wrong way to frame this, so let me clarify. The step I care about doesn't need a deprecation at all:

class MiddlewareMixin:
    sync_capable = True

    @classproperty
    def async_capable(cls):
        return cls.__acall__ is not MiddlewareMixin.__acall__

If a subclass doesn't override __acall__(), it's marked as sync-only, and BaseHandler.load_middleware() groups it with the neighboring sync middleware. That's the same fix #37177 applied to the built-in middleware, extended to third-party subclasses. Subclasses that do override __acall__(), or set async_capable explicitly, keep their current behavior.

It would also cover logging. The switches would go through adapt_method_mode(), so they'd show up in the existing "Asynchronous handler adapted" debug logs without any new logging.

Deprecating the default __acall__() would be a separate step, and I agree that one should go to the forum first. If it's eventually removed, the note in the docs about the performance trade-off could go too, since overriding __acall__() on top of sync hooks rarely makes sense.

refs

• ​https://github.com/django/django/pull/21523/changes#r3444850276

[NAYAN SHARMA]

Hi,

I'd like to contribute to this ticket. I read the discussion and the related PR #21523.

My understanding is that the proposed approach is to let BaseHandler.load_middleware() handle sync/async adaptation for MiddlewareMixin subclasses that don't override __acall__(), so the existing adaptation logging also covers them.

Before I start implementing this, could you confirm whether this approach is preferred for the ticket, or whether you'd like the change to be limited to adding logging inside MiddlewareMixin.__acall__()?

[Carlton Gibson]

I'm assuming the move back to Unreviewed was accidental.

[Mykhailo Havelia]

Replying to Carlton Gibson:

I'm assuming the move back to Unreviewed was accidental.

It was deliberate. The ticket is about performance, and Sarah only approved fixing the logging part 🙂 I've reframed the solution a bit and stepped back from the deprecation, so now I’m waiting for a response.