Opened 3 years ago
Closed 3 years ago
#33495 closed Cleanup/optimization (fixed)
"Synchronous middleware ... adapted" is not logged for synchronous middleware
Reported by: | Aaron Chong | Owned by: | Aaron Chong |
---|---|---|---|
Component: | Core (Other) | Version: | 4.0 |
Severity: | Normal | Keywords: | async |
Cc: | Andrew Godwin | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This is a bug in log messages that contradicts the guide in a warning in the docs; the feature works.
The guide at https://docs.djangoproject.com/en/4.0/topics/async/#async-views, emphasis mine:
Warning
You will only get the benefits of a fully-asynchronous request stack if you have no synchronous middleware loaded into your site. If there is a piece of synchronous middleware, then Django must use a thread per request to safely emulate a synchronous environment for it.
Middleware can be built to support both sync and async contexts. Some of Django’s middleware is built like this, but not all. To see what middleware Django has to adapt, you can turn on debug logging for the django.request logger and look for log messages about “Synchronous middleware … adapted”.
The test for "Synchronous middleware ... adapted" is instead testing an async middleware over a sync method: https://github.com/django/django/blob/7ca7f4495ba746279b734695a8dd137bf7ee0bab/tests/middleware_exceptions/tests.py#L222-L234
@override_settings(MIDDLEWARE=[ 'middleware_exceptions.middleware.async_payment_middleware', ]) def test_async_middleware(self): with self.assertLogs('django.request', 'DEBUG') as cm: response = self.client.get('/middleware_exceptions/view/') self.assertEqual(response.status_code, 402) self.assertEqual( cm.records[0].getMessage(), "Synchronous middleware " "middleware_exceptions.middleware.async_payment_middleware " "adapted.", )
About the existing implementation:
BaseHandler.load_middleware
passes the middleware name asname
toBaseHandler.adapt_method_mode
: https://github.com/django/django/blob/98ad327864aed8df245fd19ea9d2743279e11643/django/core/handlers/base.py#L53-L57
# Adapt handler, if needed. adapted_handler = self.adapt_method_mode( middleware_is_async, handler, handler_is_async, debug=settings.DEBUG, name='middleware %s' % middleware_path, )
BaseHandler.adapt_method_mode
adapts themethod
and treatsname
as the method name rather than the middleware name; when the middleware name is used, it implies a method has been adapted for the middleware, not that the middleware was adapted:
if debug and not name: name = name or 'method %s()' % method.__qualname__ if is_async: if not method_is_async: if debug: logger.debug('Synchronous %s adapted.', name) return sync_to_async(method, thread_sensitive=True) elif method_is_async: if debug: logger.debug('Asynchronous %s adapted.', name) return async_to_sync(method)
Proposed fix:
Handle middleware name
and method name separately within BaseHandler.adapt_method_mode
:
def adapt_method_mode( self, is_async, method, method_is_async=None, debug=False, name=None, ): """ Adapt a method to be in the correct "mode": - If is_async is False: - Synchronous methods are left alone - Asynchronous methods are wrapped with async_to_sync - If is_async is True: - Synchronous methods are wrapped with sync_to_async() - Asynchronous methods are left alone """ + method_name = None if method_is_async is None: method_is_async = asyncio.iscoroutinefunction(method) if debug and not name: - name = name or 'method %s()' % method.__qualname__ + method_name = 'method %s()' % method.__qualname__ if is_async: if not method_is_async: if debug: - logger.debug('Synchronous %s adapted.', name) + if name: + logger.debug('Asynchronous %s adapted.', name) + else: + logger.debug('Synchronous %s adapted.', method_name) return sync_to_async(method, thread_sensitive=True) elif method_is_async: if debug: - logger.debug('Asynchronous %s adapted.', name) + if name: + logger.debug('Synchronous %s adapted.', name) + else: + logger.debug('Asynchronous %s adapted.', method_name) return async_to_sync(method)
Change History (4)
comment:1 by , 3 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Bug → Cleanup/optimization |
comment:3 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
OK, pending Andrew's comment, I'm going to accept this as a potential cleanup.
It's the wrapped handler that's adapted to the wrapping middleware. So, I agree that the existing message perhaps implies the wrong thing:
(in
./runtests.py middleware_exceptions.tests.MiddlewareSyncAsyncTests.test_async_middleware
)Here it's the (sync)
BaseHandler._get_response()
that's actually adapted, viareturn sync_to_async(method, thread_sensitive=True)
(ln119).Tweaking the message to something like,
Synchronous handler adapted for middleware_exceptions.middleware.async_payment_middleware middleware.
would be perhaps clearer 🤔(I'd like to keep the messaging consistent on each branch — i.e. not mixing
Synchronous
andAsynchronous
in the messages — as that lets you search for the log message without doubt as to which branch you're on, even if we can improve the exact wording.)