Async-std: [tracking] sync

[yoshuawuyts]

This is a tracking issue for feature parity with std::sync. Not everything needs to be ported, but this is a full overview of what's in std::sync and async_std::sync.

@stjepang could you take a look and cross off what's not needed? I think we have most of it except maybe Barrier, Condvar, and Arc. Thanks!

Structs

• [x] sync::Arc
• [ ] sync::Condvar
• [x] sync::Mutex
• [x] sync::Barrier
• [x] sync::BarrierWaitResult
• [x] sync::MutexGuard
• [ ] sync::Once
• [x] sync::PoisonError (not required)
• [x] sync::RwLock
• [x] sync::RwLockReadGuard
• [x] sync::RwLockWriteGuard
• [ ] sync::WaitTimeoutResult
• [x] sync::Weak

Enums

• [x] sync::TryLockError (not required)

[tmccombs]

I could take a look at Condvar and maybe Once

[yoshuawuyts]

@tmccombs that'd be great!

[nbdd0121]

I looked into this briefly when trying to improve Mutex. I think it should be fairly easy: for Condvar, we simply store a list of Wakers, and notify_one or notify_all will wake one or more of these wakers. Dropping the future removes its own wakers from the list. The important thing about sync Condvar is that it unlocks and sleep atomically, which requires OS assistance usually, but in async case we don't.

For the wait with timeout APIs, it's basically async_std::future::timeout.

[tmccombs]

I think I'm pretty close (See https://github.com/async-rs/async-std/pull/369).

However, my implementation of wait_timeout doesn't work quite as expected, because if the timeout triggers, then when we poll the condvar future, it thinks it is a regular notify, so we return false for "timed_out". The easiest fix for that is to change the order of the polls for the wait_timeout to check the delay future first. But then if both futures notify the waker close enough together it will look like we timed out, even though we might not have.

Another option is to keep an atomic bool with each waker to keep track of if that waker has been woken by a call to notify. But that seems kind of heavy for this.

[nbdd0121]

I think it should be a regular notify. Condvar timeouts are never meant to be precise. If we prioritise timeout to a regular notify, then we may lose a notify_one call if we are notifying an user that is very close to timeout, which isn't really desirable.

I looked at your implementation and have a few worries:

• The implementation seems not robust against repeat polls (poll again before waker called, which is possible in cases like using either!)
• In the current implementation we can still lose a notify_one call if we receive notify_one after we decided to return timeout, but before the destructor remove the waker from the blocker list.

[ghost]

I wonder how useful wait_timeout is since futures have baked-in support for timeouts. For example, TcpStream in the standard library has method read_timeout but we don't have it in async-std because we have futures combinators for timeouts.

Maybe we can just omit wait_timeout and only implement wait?

[nbdd0121]

I wonder how useful wait_timeout is since futures have baked-in support for timeouts. For example, TcpStream in the standard library has method read_timeout but we don't have it in async-std because we have futures combinators for timeouts.

Maybe we can just omit wait_timeout and only implement wait?

I think it should be possible provided that notify_one does not get lost when the context it awoken drops the future instead of polling it (probably just need to redirect it to another waker).

[tmccombs]

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