Repository navigation
Add a new internal thread safe handle for asyncio #128340
Description
loop.call_soon_threadsafe 1 is a thread safe API to schedule callbacks from other threads however the handle returned by it i.e. asyncio.Handle 2 isn't thread safe like its cancel 3 method. This is an issue even with the GIL because thread switching is preemptive and is a bigger issue for free-threading builds.
Proposal
Add a new _ThreadSafeHandle which inherits from asyncio.Handle but internally uses a mutex to avoid multiple threads cancelling and running the callback at the same time for callbacks scheduled by loop.call_soon_threadsafe API, other APIs such as loop.call_soon will continue to use the old asyncio.Handle.
Linked PRs
- gh-128340: make asyncio.events.Handle.cancel() atomic #128347
- gh-128340: thread safe handle for
loop.call_soon_threadsafe#128369
Activity
graingert commentedon Dec 30, 2024
do you have a use-case for cancelling a call_soon_threadsafe from another thread?
graingert commentedon Dec 30, 2024
It also looks like you don't need a mutex, you could just combine the "self._cancelled" and "self._callback" attributes into one attribute where a None callback causes the handle to be considered cancelled.
kumaraditya303 commentedon Dec 30, 2024
It's not about use case, this API has been published and this should work in both free-threading and normal builds.
graingert commentedon Dec 30, 2024
Ah I see, I'll make a PR that makes this threadsafe without a mutex
kumaraditya303 commentedon Dec 30, 2024
If you combine the two, won't there be race when event loop would check if callback is None and another thread setting it to None?
What about the following sequence without mutex:
handle._callback == Noneis Falseloop.call_soon_threadsafecancels ithandle._callback == Noneis Truepythongh-128340: make asyncio.events.Handle.cancel() atomic
graingert commentedon Dec 30, 2024
this won't happen because the eventloop calls handle.run() so we can check cancellation here.
If there's a race condition some extra debug code (the 100ms debug callback timer) but won't crash
kumaraditya303 commentedon Dec 30, 2024
I saw your PR, it adds quite a lot of code for the common case when thread safety is not required, specifically you are adding unnecessary comparison and tuple packing/unpacking to the hot path of
handle._run.FWIW the handle API is very performance sensitive as all the callbacks are called thorough it, I worry this could cause a significant performance loss for no real benefit.
graingert commentedon Dec 30, 2024
You'll need the mutex in the hot path anyway, although only for call_soon_threadsafe it will change the code quite drastically anyway. Probably it's worth benchmarking the approaches
I can replace the tuple unpacking for index lookups instead, and see if that's faster
asvetlov commentedon Dec 30, 2024
I honestly prefer the @kumaraditya303 idea:
graingert commentedon Dec 31, 2024
how about we:
I don't think it's possible to use a mutex with call_soon_threadsafe, otherwise you risk cancel() not working when it's re-entrant eg due to calling handle.cancel() in a signal handler
asvetlov commentedon Dec 31, 2024
Doesn't
threading.RLockexist for reentrant calls? Did I miss something?graingert commentedon Dec 31, 2024
threading.RLock will allow the code to be re-entrant, but then the critical section can be occupied by two contexts
kumaraditya303 commentedon Dec 31, 2024
In general it's not safe to call any asyncio API directly from signal handlers, you should call your code using
loop.call_soon_threadsafefrom signal handlers. For example see #122938graingert commentedon Dec 31, 2024
yes, but we're talking about call_soon_threadsafe
kumaraditya303 commentedon Dec 31, 2024
If you want to cancel a handle, why not call
loop.call_soon_threadsafe(handle.cancel)from signal handler?gh-128340: add thread safe handle for `loop.call_soon_threadsafe` (#1…
kumaraditya303 commentedon Jan 6, 2025
Added thread safe handle in #128369 with tests, I'll reopen if there are more thread safety issues I find with this but this is done for now.
pythongh-128340: add thread safe handle for `loop.call_soon_threadsaf…