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

[tracking] sync #217

Open
yoshuawuyts opened this issue Sep 19, 2019 · 8 comments

Comments

@yoshuawuyts
Copy link
Member

commented Sep 19, 2019

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

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

Enums

  • sync::TryLockError (not required)
@yoshuawuyts yoshuawuyts added this to the std parity milestone Sep 19, 2019
bors bot added a commit that referenced this issue Sep 19, 2019
Merge #218
218: expose sync::{Arc,Weak} r=stjepang a=yoshuawuyts

Ref #217. Exposes `std::sync::Arc` and `std::sync::Weak`. Thanks!

Co-authored-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
@tmccombs

This comment has been minimized.

Copy link

commented Oct 11, 2019

I could take a look at Condvar and maybe Once

@yoshuawuyts

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2019

@tmccombs that'd be great!

@nbdd0121

This comment has been minimized.

Copy link

commented Oct 18, 2019

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 added a commit to tmccombs/async-std that referenced this issue Oct 18, 2019
@tmccombs

This comment has been minimized.

Copy link

commented Oct 18, 2019

I think I'm pretty close (See #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

This comment has been minimized.

Copy link

commented Oct 18, 2019

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.
@stjepang

This comment has been minimized.

Copy link
Member

commented Oct 18, 2019

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

This comment has been minimized.

Copy link

commented Oct 18, 2019

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

This comment has been minimized.

Copy link

commented Oct 18, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.