-
Notifications
You must be signed in to change notification settings - Fork 401
fix: reporter context retrieval #5202
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
fix: reporter context retrieval #5202
Conversation
baszalmstra
left a comment
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.
Yes this doesnt seem right. Instead we should just cancel the child task as well because most likely the parent errored or was itself cancelled.
I've added a child cancellation token ( from parent token ). |
|
@nichmor When you have time can you point me to how this now fixes the issue. I dont understand why this would fix the issue. From my perspective the tasks were already cancelled anyway? The child cancellation tokens are also never cancelled manually are they? |
Description
Assuming this might fix #5200
How Has This Been Tested?
I've been testing this with the https://github.com/ruben-arts/ros_workspace as that spawns multiple backends, and it was the only project I could get the error with. I've not been able to recreate the race condition, but they are tricky so this is not a promised fix.
I've extended the stress test by making it multi-env, with pypi sdist and solving for multiple platforms:
I can't seem to break it anymore, but it was hard in the first place, so 🤞
AI Disclosure
Tools: Claude
Checklist:
This type of issue is almost impossible to recreate in a test as it's flaky behavior.