-
Notifications
You must be signed in to change notification settings - Fork 72
fix(runtime): only remove active task's Waker by DropHook
#502
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 refactors the scheduler's task cleanup mechanism to improve efficiency and correctness. Instead of draining and consuming wakers during cleanup, the code now uses non-consuming iterations and relies on drop hooks to properly clean up waker references.
- Changed the
clear()method to usewake_by_ref()with iteration instead ofwake()with drain - Replaced
try_remove()withremove()in the drop hook - Improved documentation comments for clarity
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Look familiar? Things like this (TOCTOU) are happening all over the galaxy (really), right now! |
|
Could you add a test for this fix? |
tbh, I don’t know how to write this test case, because the previous situation did not cause a memory leak or a panic, It was just a sudden realization that the logic wasn’t entirely correct. |
|
Almost LGTM. @George-Miao to review it too. |
How about this: add two tasks to the runtime, and the second one panics after being woke. If there's a bug, the second one might be removed and never panics. |
Not fully understand, in |
|
You're right. Let's wait for another reviewer:) |
|
Exact problem with slab I described in the first PR lol |
|
Could you bump the version so that I can publish a fix release? |
Done, version bumped. |
Berrysoft
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
This PR fixes a rare issue when running
Scheduler::clear().Currently,
Scheduler::clear()is to first drainactive_tasksand wake theirWakers, and then drop all futures. However, a future might spawn a new task during its drop. In that case, a newWakergets inserted intoactive_tasks, but its index might still be used by another task due toactive_taskshaving been drained and the index being reused.As a result, the drop hook of the original task could remove a
Wakerthat does not belong to it, which is not the behavior we expect.