Add basic support for async dispatch #1771
Conversation
|
Python 3.7 doesn't have |
|
I merged main into this branch (after merging #1773); that should get the tests passing again. |
|
Looks like whitespace code style errors because I did the work on my not-quite correctly configured personal machine. |
|
|
||
| dispatch_same(handler, event) | ||
|
|
||
| await asyncio.sleep(0.1) |
There was a problem hiding this comment.
Can we rework this (and similar tests) to wait on an event instead? That is, create an event with handled = asyncio.Event(), then do a handled.set() within the handler, and replace this line with something like await asyncio.wait(handled.wait(), SOME_TIMEOUT). Then SOME_TIMEOUT can be quite large (e.g., 60 seconds), since we don't expect to hit the timeout in a normal test run.
traits/tests/test_observe.py
Outdated
|
|
||
| @observe('value') | ||
| async def value_changed_async(self, event): | ||
| await asyncio.sleep(0) |
There was a problem hiding this comment.
What's the purpose of the sleep here?
There was a problem hiding this comment.
My understanding is that this yields execution to the event loop - originally I had a longer sleep here, but less than the sleep in the test. It may not be needed - but I would like the async function to be actually doing something async-y before it sets the trait.
| from traits.observation._observe import add_or_remove_notifiers | ||
| from traits.observation.expression import compile_expr | ||
|
|
||
| #: Set to hold references to active async traits handlers. | ||
| _active_handler_tasks = set() |
There was a problem hiding this comment.
I'm wondering whether we need this to be thread-local, given that different threads might have different current event loops. It's probably okay to have multiple threads modifying a single set simultaneously, if there's no overlap in the actual elements being added / removed. Not sure whether that's still going to be true in free-threaded Python 3.13, though. (OTOH, we've got significant work to do if we ever want to make Traits compatible with free-threading.)
There was a problem hiding this comment.
I think this is fine - each object in the set is independent and there should never be the same task created twice in different threads. I can see a problem if somehow the task gets finished before the callback is added, but I don't know if that is possible.
There was a problem hiding this comment.
each object in the set is independent and there should never be the same task created twice in different threads
Agreed, but the set structure itself would be shared between calls. Right now I think the GIL protects us, but that may eventually no longer be true.
I can see a problem if somehow the task gets finished before the callback is added, but I don't know if that is possible.
Yep, I don't think that's possible. The task can't start executing until we hit a yield point, so it won't even start until after the callback is added.
There was a problem hiding this comment.
Agreed, but the set structure itself would be shared between calls. Right now I think the GIL protects us, but that may eventually no longer be true.
Not sure exactly what the free-threading folks are proposing, but if threaded code needs to worry about adding locks around shared containers at the Python level, it probably won't get adopted widely. But if it does there'll be plenty of warning I suspect.
Co-authored-by: Mark Dickinson <[email protected]>
|
Looks like |
|
All the twisty ins and outs of different Python versions accounted for, I think. Would feel more comfortable if we tested the async handlers actually doing something async-y, so may add something to the tests to do that. |
|
Ready for a re-review. |
| with self.asyncio_exception_handler(exception_handler): | ||
| with self.assertRaises(asyncio.exceptions.TimeoutError): | ||
| dispatch_same(handler, event) | ||
| await asyncio.wait_for(event.wait(), timeout=0.1) |
There was a problem hiding this comment.
I adjusted this slightly in cc9127c to move the event to the exception handler, which then gives us something to wait on.
This adds basic support for async coroutines as trait observe handlers, as discussed in #1770
This is designed to be the smallest thing that will work.
Checklist
docs/source/traits_api_reference) -- don't think there are any changes heredocs/source/traits_user_manual)