Make show toolbar callback function async/sync compatible.#2066
Make show toolbar callback function async/sync compatible.#2066tim-schilling merged 17 commits intomainfrom
Conversation
This checks if the SHOW_TOOLBAR_CALLBACK is a coroutine if we're in async mode and the reverse if it's not. It will automatically wrap the function with sync_to_async or async_to_sync when necessary.
for more information, see https://pre-commit.ci
|
I can confirm these changes completely fix the issue reported in #2061, both when using a synchronous callback or an asynchronous one! |
|
I understand the problem and this solution work. I don't like very much to for the use sync_to_async all the time. We lock in the thread the function and it's not, for me, a good practice.
We ask to dev to write a sync or/and async version of their function. And if they won't it will work. But we let the possibility to async dev to do what they want. |
To be clear, if you're using a sync callback with Is that what you understood @elineda? |
yeah I missed that. In that case we can't have both sync and async fonction in the same time, but for a dev dependencies I think it's ok. ok for me |
| from debug_toolbar.middleware import get_show_toolbar | ||
|
|
||
| show_toolbar = get_show_toolbar() | ||
| show_toolbar = get_show_toolbar(async_mode=False) |
There was a problem hiding this comment.
Shouldn't we pass async_mode based on the type of request WSGIRequest or ASGIRequest here ? I mean I am not sure whether require_show_toolbar would ever be exposed to a custom async view in async context or vice versa but this would save the conversion as it slows down the overall process. thoughts?
There was a problem hiding this comment.
I was hoping require_show_toolbar was an undocumented API, but it is documented. Yes, we need to pass in a better value than always False. Good catch @salty-ivy!
It feels like we'd want async_mode defined by whether view is a coroutine though rather than based on WSGIRequest vs ASGIRequest. Not 100% sure though.
There was a problem hiding this comment.
It can possible break in a situation where everything is in async context
including show_toolbar call back and if I use require_show_toolbar on an async view it will forcefully convert it into a sync func and we would get into similar error given that require_show_toolbar is exposed to such conditions :D
There was a problem hiding this comment.
It feels like we'd want
async_modedefined by whetherviewis a coroutine though rather than based on WSGIRequest vs ASGIRequest. Not 100% sure though.
may be checking both but it seems like that would cause a rabbit hole : (
There was a problem hiding this comment.
Oh. I totally missed that this decorator needs to be async friendly.
There was a problem hiding this comment.
It feels like we'd want
async_modedefined by whetherviewis a coroutine though rather than based on WSGIRequest vs ASGIRequest. Not 100% sure though.
This is correct, we should check whether view is a coroutine or not since django also converts the request object based on that, just checked : D
There was a problem hiding this comment.
yeah missed that too and yeah a iscoroutinefunction is the way other django decorator work.
|
@tim-schilling Any idea why tox tests are failing in all the recent PRs ? |
|
@salty-ivy The reason for this is #2082 , I have now pushed a change pinning django-csp to versions before 4 for now. This allows us to continue working on everything else. Please resubmit the PR (merge main and push again) so that tests have a chance to run again? Thank you! |
|
@matthiask Thanks! @tim-schilling @elineda I think this is ready for another review. |
|
I have also pushed documentation changes Its taking some time to reflect on github :D |
…mmons#2066) This checks if the SHOW_TOOLBAR_CALLBACK is a coroutine if we're in async mode and the reverse if it's not. It will automatically wrap the function with sync_to_async or async_to_sync when necessary. * ASGI check approach with added test and docs * async compatible require_toolbar and tests * add docs for async require_toolbar --------- Co-authored-by: Aman Pandey <aman2001mi@gmail.com>
5 participants
Description
This checks if the SHOW_TOOLBAR_CALLBACK is a coroutine if we're in async mode and the reverse if it's not. It will automatically wrap the function with sync_to_async or async_to_sync when necessary.
Fixes #2061
Checklist:
docs/changes.rst.