Skip to content
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

Implement async_std::sync::Condvar #369

Merged
merged 8 commits into from
Apr 12, 2020
Merged

Conversation

tmccombs
Copy link
Contributor

Part of #217

@tmccombs tmccombs mentioned this pull request Oct 18, 2019
14 tasks
@nbdd0121
Copy link
Contributor

@nbdd0121 do you have any suggestion on how to resolve your concerns?

I think notifying another waker when a WaitFuture is dropped with its opt_waker set to None should be enough. This should handle the notify_one well, but it may cause extra tasks being notified if we
have the following case:

  • Task A calls wait
  • Task B calls notify_all
  • Task C calls wait
  • Task A cancels wait

Task C will ends up being waken up. However I don't think it's a huge problem, because std's Condvar mentions that:

Note that this function is susceptible to spurious wakeups. Condition variables normally have a boolean predicate associated with them, and the predicate must always be checked each time this function returns to protect against spurious wakeups.

@nbdd0121
Copy link
Contributor

IMO we don't need has_blocked. Unlike Mutex which assumes most calls don't block, for Condvar I think we can assume there are tasks blocking when notify_one or notify_all is called.

@roblabla
Copy link

roblabla commented Nov 1, 2019

I feel like the wait/wait_timeout are kinda big footguns. If I understand them properly, the returned future will always return Ready the second time they're polled, without necessarily being actually notified. This can happen quite easily by using e.g. the join adapter from futures-rs.

This is technically okay according to Condvars' spurious wakeup conditions, but that is not mentioned at all in the docs. Besides, it seems very trivial to cause a spurious wakeups here compared to on a "normal" blocking implementation, and will likely produce bad, hard to debug code. I kinda question the usefulness of those functions compared to wait_until.

@tmccombs
Copy link
Contributor Author

tmccombs commented Nov 1, 2019

If I understand them properly, the returned future will always return Ready the second time they're polled

Kind of. They will return Ready if they are polled and the attached Mutex is available. I could potentially use an Arc<AtomicBool> to keep track of if it was actually notified. But I think (though I don't have benchmarks) that in most cases that would hurt performance.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Nov 3, 2019

Kind of. They will return Ready if they are polled and the attached Mutex is available. I could potentially use an Arc<AtomicBool> to keep track of if it was actually notified. But I think (though I don't have benchmarks) that in most cases that would hurt performance.

Probably only need to check if Option<Waker> is turned to None. Otherwise it'll be spurious wake up.

@tmccombs
Copy link
Contributor Author

tmccombs commented Nov 4, 2019

Probably only need to check if Option is turned to None. Otherwise it'll be spurious wake up.

Yes, but that requires locking the mutex for the wakers when polled, which is doable, but could hurt performance if the condition variable has very much contention. And if wait_until/wait_timeout_until are used, then it isn't an issue.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Nov 4, 2019

You need to lock the mutex to remove the waker when cancelling/completing the notify call, or to insert the waker when beginning a new call. I don't see why it'll hurt performance compared to wait_until.

@tmccombs
Copy link
Contributor Author

tmccombs commented Nov 4, 2019

Ok, I'll take a stab at implementing that. It looks like there is also now a WakerSet type that I should use instead of a Slab of Wakers.

@tmccombs
Copy link
Contributor Author

tmccombs commented Nov 4, 2019

@nbdd0121 I think my latest push should address your concerns.

@yoshuawuyts yoshuawuyts requested a review from a user November 9, 2019 12:11
@nbdd0121
Copy link
Contributor

WakerSet's notify_one does not wake up anything if some task is woken already. This is slightly different from Condvar's notify_one which should always wake up one task (in the case of two concurrent notify_one call for example).

@tmccombs
Copy link
Contributor Author

If i understand the code on master that is how notify_any behaves, but notify_one behaves as we want for condvar.

@nbdd0121
Copy link
Contributor

Sorry, you're right. That change is introduced in e9edadf which isn't part of a PR so I am not aware of.


impl fmt::Debug for Condvar {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
//f.debug_struct("Condvar").finish()
Copy link
Member

Choose a reason for hiding this comment

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

what about this comment?

Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

Some small things and needs a rebase, but other than that I think this looks good and we should really get this in, it is quite a useful primitive

src/sync/condvar.rs Show resolved Hide resolved
src/sync/waker_set.rs Show resolved Hide resolved
This should also address concerns about spurious wakeups.
And remove an unnneded comment in a Debug implementation
@tmccombs
Copy link
Contributor Author

I rebased and addressed your comments. Should I squash as well?

@dignifiedquire dignifiedquire merged commit db438ab into async-rs:master Apr 12, 2020
@dignifiedquire
Copy link
Member

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants