Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use a canary task to detect asyncio.run shutdown - and join selector thread there #3173

Closed
graingert opened this issue Aug 8, 2022 · 4 comments · Fixed by #3272
Closed
Labels

Comments

@graingert
Copy link
Contributor

graingert commented Aug 8, 2022

I was looking at trying to shutdown the selector thread when using asyncio.run, and I think it's possible by starting a task that sleeps until it is cancelled, and then loses the reference to it.

This way there's only two ways it can terminate, by asyncio.run using _cancel_all_tasks and loop.close() removing the sleep timer handle:

async def _canary_task(selector_thread_wr):
    try:
        while True:  # uvloop clams sleeps to 1000 years, and someone could fast-forward the uv clock
             await asyncio.sleep(math.inf)
    except asyncio.CancelledError:
        # asyncio.run is calling _cancel_all_tasks - join selector thread here
        selector_thread = selector_thread_wr()
        if selector_thread is not None:
             # notify selector thread and join it
            
    except GeneratorExit:
        # loop.close() was called without _cancel_all_tasks notify selector thread to shutdown but don't join it
@bdarnell
Copy link
Member

I have to admit that it took me a while to see what the issue is - asyncio.run closes its loop, and we join the thread in AddThreadSelectorEventLoop.close, so what's the problem? Of course the wrapper AddThreadSelectorEventLoop isn't the same object as the one used by asyncio.run, d'oh!

This seems like a reasonable approach; it's probably the best we can do without global changes (i.e. changing the policy so that asyncio.run constructs an AddThreadSelectorEventLoop).

@bdarnell
Copy link
Member

bdarnell commented May 7, 2023

Coming back to this after a while, and one thing's not clear: why does CancelledError notify and join, while GeneratorExit notifies without joining? Shouldn't we be able to join after notification either way?

bdarnell added a commit to bdarnell/tornado that referenced this issue May 17, 2023
Attempt to clean up our selector thread (on windows) when
the event loop shuts down without waiting for GC. We can't
always do this, but we can at least detect it for asyncio.run.
For cases where we do need to rely on GC, we now rely on the
GC of the canary Task instead of the AddThreadSelectorEventLoop.

Fixes tornadoweb#3173
bdarnell added a commit to bdarnell/tornado that referenced this issue May 29, 2023
Attempt to clean up our selector thread (on windows) when
the event loop shuts down without waiting for GC. We can't
always do this, but we can at least detect it for asyncio.run.
For cases where we do need to rely on GC, we now rely on the
GC of the canary Task instead of the AddThreadSelectorEventLoop.

Fixes tornadoweb#3173
@bdarnell
Copy link
Member

Having spent some time on this, I'm seeing that it's difficult to clean up the thread cleanly without also producing warnings about un-awaited tasks or tasks destroyed while they are pending.

In the GC/GeneratorExit case, it seems very difficult to escape the warnings logged when a pending task is GC'd. The problem is that I haven't figured out how to have a task in a position to catch a GeneratorExit without logging a warning even when things are shut down cleanly.

The asyncio.run/CancelledError case is more frustrating. It seems like we should be able to catch the CancelledError cleanly to shut down when asyncio.run is used, with no side effects if it is not. But I haven't been able to come up with a solution that works.

#3272 currently passes its tests, meaning it cleans up its threads. But it logs spurious warnings, and there's no good way to silence them. It's better to log a warning than to leak a thread, and windows isn't a fully-supported platform, but it's still ugly to have that warning all the time.

Maybe this is a good reason to clean up the SelectorThread code and submit it upstream to cpython for 3.13. There the cleanup would be unambiguous.

bdarnell added a commit to bdarnell/tornado that referenced this issue Jun 19, 2023
Async generators have a special shutdown protocol which allows
us to detect the end of the event loop and stop our thread.
This lets us clean up the thread reliably when the event loop
is started/stopped via the tornado IOLoop interfaces (which
explicitly know about the selector thread), or when the
latest asyncio interfaces are used (asyncio.run or manually
calling shutdown_asyncgens).

The thread is still leaked when older versions of the asyncio
interfaces are used (loop.close *without* shutdown_asyncgens), but
I've been unable to find a solution that does not print leak warnings
even in the event of a clean shutdown. Use of shutdown_asyncgens is
now effectively required for apps combining asyncio and tornado.
This is unfortunate since leaking a thread is relatively expensive
compared to the usual consequences of failing to call
shutdown_asyncgens, but it seems to be the best we can do.

Fixes tornadoweb#3173
bdarnell added a commit to bdarnell/tornado that referenced this issue Jun 19, 2023
Async generators have a special shutdown protocol which allows
us to detect the end of the event loop and stop our thread.
This lets us clean up the thread reliably when the event loop
is started/stopped via the tornado IOLoop interfaces (which
explicitly know about the selector thread), or when the
latest asyncio interfaces are used (asyncio.run or manually
calling shutdown_asyncgens).

The thread is still leaked when older versions of the asyncio
interfaces are used (loop.close *without* shutdown_asyncgens), but
I've been unable to find a solution that does not print leak warnings
even in the event of a clean shutdown. Use of shutdown_asyncgens is
now effectively required for apps combining asyncio and tornado.
This is unfortunate since leaking a thread is relatively expensive
compared to the usual consequences of failing to call
shutdown_asyncgens, but it seems to be the best we can do.

Fixes tornadoweb#3173
@bdarnell
Copy link
Member

#3272 is now merged, which detects the closure of the event loop with an async generator and cleans up the thread. This means that we clean things up reliably when asyncio.run, asyncio.Runner, or event_loop.shutdown_asyncgens are used, although we still have a thread leak if none of those are used (as in the original pre-3.6 versions of asyncio)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants