Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add basic support for async dispatch #1771
Add basic support for async dispatch #1771
Changes from 3 commits
2c27ff1
2586860
d1879e9
e886ff7
1358645
8861b6a
1df0bfc
cf8c39d
4053887
940b371
9148d03
cc9127c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rework this (and similar tests) to wait on an event instead? That is, create an event with
handled = asyncio.Event()
, then do ahandled.set()
within the handler, and replace this line with something likeawait asyncio.wait(handled.wait(), SOME_TIMEOUT)
. ThenSOME_TIMEOUT
can be quite large (e.g., 60 seconds), since we don't expect to hit the timeout in a normal test run.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of the
sleep
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.