Make show toolbar callback function async/sync compatible.

[tim-schilling]

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:

• I have added the relevant tests for this change.
• I have added an item to the Pending section of docs/changes.rst.

[adamantike]

I can confirm these changes completely fix the issue reported in #2061, both when using a synchronous callback or an asynchronous one!

[elineda]

I understand the problem and this solution work.
BUT :D

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.

• I would like to create a setting: SHOW_ASYNC_TOOLBAR_CALLBACK
• In the call we check before a SHOW_TOOLBAR_CALLBACK and if it's none check SHOW_ASYNC_TOOLBAR_CALLBACK with a decorator.
• In the acall we check before a SHOW_ASYNC_TOOLBAR_CALLBACK and if it's none check SHOW_TOOLBAR_CALLBACK with a decorator.

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.

[tim-schilling]

I don't like very much to for the use sync_to_async all the time

To be clear, if you're using a sync callback with async_mode=False, it doesn't wrap it. Similarly, if you're using an async callback with async_mode=True, it doesn't wrap it. It only wraps if the callback is written for the opposite of async_mode.

Is that what you understood @elineda?

[elineda]

I don't like very much to for the use sync_to_async all the time

To be clear, if you're using a sync callback with async_mode=False, it doesn't wrap it. Similarly, if you're using an async callback with async_mode=True, it doesn't wrap it. It only wraps if the callback is written for the opposite of async_mode.

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

[salty-ivy]

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?

[tim-schilling]

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.

[salty-ivy]

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

[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.

may be checking both but it seems like that would cause a rabbit hole : (

[tim-schilling]

Oh. I totally missed that this decorator needs to be async friendly.

[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.

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

[elineda]

yeah missed that too and yeah a iscoroutinefunction is the way other django decorator work.