django / asgiref

ASGI specification and utilities
https://asgi.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
1.46k stars 207 forks source link

async_to_sync does not use the correct thread when used as decorator #438

Open germaniuss opened 7 months ago

germaniuss commented 7 months ago

When using a function decorated with @async_to_sync inside of a function decorated with @sync_to_async a new thread is spawned instead of reusing the same thread and event loop. This issue does not happen when doing async_to_sync(coroutine)().

This is due to the fact that the AsyncToSync sets main_event_loop and main_event_loop_pid on __init__ and not on __call__ and SyncToAsync only sets main_event_loop and main_event_loop_pid on __call__.

The easy way to solve this is moving the initialization of self.main_event_loop and self.main_event_loop_pid from __init__ to __call__ in AsyncToSync (#439 fixes the issue)

Consider this example on why this is relevant:

import asyncio
from asgiref.sync import sync_to_async, async_to_sync
import threading

# this function cannot be changed
def lib_call(callback):
    callback.callback()

class Callback:
    def callback():
        async def coro():
            print(f"Callback thread: {threading.get_ident()}")
            # async code here
        async_to_sync(coro)()

class CallbackDecorated:
    @async_to_sync   
    async def callback():
        print(f"Callback decorated thread: {threading.get_ident()}")
        # async code here

@sync_to_async
def blocking_func():
    print(f"Blocking func thread: {threading.get_ident()}")
    # this uses the outer event loop
    lib_call(Callback())
    # this spawns a new thread and new event loop for the callback
    lib_call(CallbackDecorated())

async def main():
    try:
        async with asyncio.TaskGroup() as tg:
            task0 = tg.create_task(blocking_func())
    except Exception as exc:
        pass

print(f"Main thread: {threading.get_ident()}")
asyncio.run(main())

The output is:

Main thread: 140545354248192
Blocking func thread: 140545298830912
Callback thread: 140545354248192
Callback decorated thread: 140544949696064

In this example calling async_to_sync in blocking_func is a bit akward, therefore our only option is to modify the callback method. This however is also akward since we have to create an auxiliary coroutine for it to use the correct thread. The best syntax would be to use the @async_to_sync decorator that is however not thread safe.

Ruman-12 commented 7 months ago

Hey @germaniuss i would like work on this issue. could you please assign me this issue?

andrewgodwin commented 7 months ago

This is interesting because I personally do a lot of async_to_sync(coroutine)() style code so I probably didn't notice this myself! PR looks good - one linting issue that needs fixing and I can land it.

germaniuss commented 7 months ago

This is interesting because I personally do a lot of async_to_sync(coroutine)() style code so I probably didn't notice this myself! PR looks good - one linting issue that needs fixing and I can land it.

There is actually one issue I spotted in the PR. In case I run the new decorated function in a thread not started by SyncToAsync, the AsyncToSync object would not have self.main_event_loop initialized, I fixed that issue by moving the check for if there is a running event loop back to __init__ (which made no sense to check in __call__ anyways).

Also fixed the linter issue.