Don’t capture the event loop in AsyncToSync.__init__

[andersk]

It was unintuitive that merely defining a function in a different location should affect which loop it uses when called.

test_async_to_sync_to_thread_decorator was the only test in the whole test suite where a captured loop made it through the top of AsyncToSync.__call__ without being replaced. This test was wrong to expect us to capture the loop across asyncio.to_thread:

asyncio.to_thread intentionally doesn’t copy the running loop to the new thread, because a loop object is not thread-safe. Granted, we only use the thread-safe subset of the loop interface. However, the user shouldn’t expect that. If they wanted to go back to the old thread, they would have written sync_to_async instead of asyncio.to_thread.

Includes the test case from #558 (comment), which now passes.

[carltongibson]

This looks interesting 👀

There'll be a small behaviour change right, which we can clarify about when to use to_thread vs sync_to_async (and thread_sensitive)

[carltongibson]

I wonder if there's a version of the deadlock that could come up along this branch, still requiring the is_running() check 🤔

Update: See 327c966 to assess.

[carltongibson]

Hey @andersk — I added 327c966 to show the concern along threadlocal loop path — can you let me know what you think?

[carltongibson]

Just fyi, I'm at PyCon Italia this week and intended to try and resolve the set of issues here and get a release out. My current thinking is to take your patch but also handle the extra case shown by the test I added (as separate commits). If you have any thoughts do let me know! Thanks again for the input!

[andersk]

Looking at your new test:

inner() runs in the main thread if the loop is still running (e.g. if asyncio.sleep(0.1) is increased to asyncio.sleep(0.5)), but your patch allows it to run in a new thread otherwise. It still seems dangerous to have code that may or may not run on the main thread depending on timing; this is a recipe for introducing thread safety bugs that are difficult to reproduce and understand.

Is your test fundamentally different from the following deadlock that doesn’t use asgiref at all? Why should adding a round trip through async_to_sync and sync_to_async shift the burden of avoiding the deadlock onto asgiref?

import asyncio
import threading

finished = threading.Event()

async def inner():
    await asyncio.sleep(0.3)
    finished.set()

async def driver():
    asyncio.ensure_future(inner())
    await asyncio.sleep(0.1)

loop = asyncio.new_event_loop()
loop.run_until_complete(driver())
assert not loop.is_running() and not loop.is_closed()
finished.wait()

Maybe it would help to see the actual pytest-asyncio example you’re investigating, if you have one.

[carltongibson]

It's more that we're introducing a change in behaviour from what we've always done here. (In the face of the reported deadlocks.) This one code path is all that remains of the possible "stopped loop" case. So... what's the correct behaviour at that point?