-
Notifications
You must be signed in to change notification settings - Fork 48
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(madsim): drop the future within the scope of TaskEnterGuard
when a task is aborted
#231
Conversation
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
TaskEnterGuard
when a task is abortedTaskEnterGuard
when a task is aborted
// cancelled task or killed node: drop the future | ||
continue; | ||
drop |
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.
Ohhhh I see. The continue
triggered us to directly proceed to the end of the loop, and call the destructor of runnable implicitly. Then it calls the drop for the struct, which spawned the thread.
Seems acceptable to me to drop it because task was cancelled / node was killed.
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 once pass CI. @wangrunji0408 could you take a look too?
I considered opposite cases like:
In such cases, we will always have task context, since their parent future will be executing with task context, and the spawning steps happen sequentially. The newly spawned futures will be handled by So I think we just need to handle the case of node kill / task abort as was done in this PR. |
Many thanks @BugenZhao! |
Signed-off-by: Bugen Zhao <[email protected]>
@@ -623,7 +630,7 @@ impl Spawner { | |||
F::Output: 'static, | |||
{ | |||
if self.info.killed.load(Ordering::Relaxed) { | |||
panic!("spawning task on a killed node"); | |||
tracing::warn!("spawning task on a killed node, the task will never 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.
Also relax this check so that spawn
during drop by killing a node won't panic.
Signed-off-by: Bugen Zhao <[email protected]>
Let's also bump the version of |
Given that this PR has been tested in risingwavelabs/risingwave#18852 and RisingWave might be the largest project to adopt madsim, I believe we can safely merge this PR now. |
Signed-off-by: Bugen Zhao [email protected]