-
Notifications
You must be signed in to change notification settings - Fork 78
fix(runtime): avoid dropping local queue on different threads #513
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
Conversation
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.
Pull Request Overview
This PR fixes an issue where task queues could be dropped on different threads than where they were created, which could cause panics due to SendWrapper's drop guard. The fix introduces a WeakTaskQueue pattern to ensure the actual TaskQueue is always dropped on its creator thread.
- Wraps
TaskQueuefields inArcto enable weak references - Introduces
WeakTaskQueuewith weak references and thread tracking for safe cross-thread handling - Refactors task scheduling to use
WeakTaskQueuein closures that may be dropped on different threads
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| compio-runtime/src/runtime/scheduler/send_wrapper.rs | Adds #[allow(dead_code)] to unused get() method |
| compio-runtime/src/runtime/scheduler/mod.rs | Refactors task queue architecture to prevent cross-thread drops by introducing WeakTaskQueue and Arc-wrapping queue components |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
George-Miao
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.
LGTM
|
I’ve opened a PR that uses raw pointers, so we don’t need extra tricks to work around the limitations of |
|
Also @Paraworker to review if you're free. |
George-Miao
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.
LGTM
|
I think I'm going to merge this one. It'll be complex to reimplement a better |
Closes #512
Not sure if it is OK.
cc: @Paraworker