Skip to content

Bug: Race condition in next_when_notified #280

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bug: Race condition in next_when_notified #280

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/buf/fixed/pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,10 @@ impl<T: IoBufMut> FixedBufPool<T> {
pin!(notified);
loop {
// In the single-threaded case, no buffers could get checked in
// between us calling `try_next` and here, so we can't miss a wakeup.
notified.as_mut().await;
// between us calling `try_next` and here. However, we may still miss a wake-up,
// as multiple check-ins can occur before any waking tasks are scheduled,
// which would result in the loss of a permit
Comment on lines 276 to +279
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is right, thanks for the clarification. In fact, the preamble of this comment can be completely replaced because it's irrelevant: multiple buffers can still be checked in while awaiting on notified before another iteration of the loop.

notified.as_mut().enable();

if let Some(data) = self.inner.borrow_mut().try_next(cap) {
// Safety: the validity of buffer data is ensured by
Expand All @@ -284,6 +286,9 @@ impl<T: IoBufMut> FixedBufPool<T> {
return buf;
}

// Await notify_one
notified.as_mut().await;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment below clarifies why the whole branch below the if ... { ... return buf; } can be taken, so part of it should be placed above this.

// It's possible that the task did not get a buffer from `try_next`.
// The `Notify` entries are created once for each requested capacity
// and never removed, so this `Notify` could have been holding
Expand Down