-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
std: Second pass stabilization of sync #20315
Conversation
r? @pcwalton (rust_highfive has picked a reviewer for you, use r? to override) |
r? @aturon |
cc @steveklabnik, the guide mentions futures and this may affect that |
3cebea3
to
5bd3650
Compare
Overall this looks good, but I'm a bit uncomfortable stabilizing any of the methods of A smaller nit is the |
5bd3650
to
ebb496e
Compare
Rebased, semaphores are now re-r? @aturon |
@@ -218,9 +215,6 @@ pub struct UnixListener { | |||
path: CString, | |||
} | |||
|
|||
unsafe impl Send for UnixListener {} | |||
unsafe impl Sync for UnixListener {} |
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.
I remember adding these implementations because some tests needed them. If you are removing them from std::sys::unix
, could you please clean up windows as well?
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.
Hm I think I was overzealous here, UnixStream
above didn't need the impls but due to CString
these are needed.
ebb496e
to
3474279
Compare
@@ -117,9 +117,6 @@ pub struct UnixStream { | |||
write_deadline: u64, | |||
} | |||
|
|||
unsafe impl Send for UnixStream {} |
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.
I think I'm missing something but, why shouldn't UnixStream
be Send
and Sync
?
Mmh why can't Am I missing something? Also, it'd be nice to use this change to make both, Unix and windows, implementations consistent. |
The windows implementations need manual |
@alexcrichton yup, it's inferred to be re-windows: fair enough. Though, I wonder if we should implement Otherwise, LGTM. |
3474279
to
22464ed
Compare
This pass performs a second pass of stabilization through the `std::sync` module, avoiding modules/types that are being handled in other PRs (e.g. mutexes, rwlocks, condvars, and channels). The following items are now stable * `sync::atomic` * `sync::atomic::ATOMIC_BOOL_INIT` (was `INIT_ATOMIC_BOOL`) * `sync::atomic::ATOMIC_INT_INIT` (was `INIT_ATOMIC_INT`) * `sync::atomic::ATOMIC_UINT_INIT` (was `INIT_ATOMIC_UINT`) * `sync::Once` * `sync::ONCE_INIT` * `sync::Once::call_once` (was `doit`) * C == `pthread_once(..)` * Boost == `call_once(..)` * Windows == `InitOnceExecuteOnce` * `sync::Barrier` * `sync::Barrier::new` * `sync::Barrier::wait` (now returns a `bool`) * `sync::Semaphore::new` * `sync::Semaphore::acquire` * `sync::Semaphore::release` The following items remain unstable * `sync::SemaphoreGuard` * `sync::Semaphore::access` - it's unclear how this relates to the poisoning story of mutexes. * `sync::TaskPool` - the semantics of a failing task and whether a thread is re-attached to a thread pool are somewhat unclear, and the utility of this type in `sync` is question with respect to the jobs of other primitives. This type will likely become stable or move out of the standard library over time. * `sync::Future` - futures as-is have yet to be deeply re-evaluated with the recent core changes to Rust's synchronization story, and will likely become stable in the future but are unstable until that time comes. [breaking-change]
Conflicts: src/libstd/rt/exclusive.rs src/libstd/sync/barrier.rs src/libstd/sys/unix/pipe.rs src/test/bench/shootout-binarytrees.rs src/test/bench/shootout-fannkuch-redux.rs
@alexcrichton not sure if you read my last comment. I'm wondering what you think about the last thing I said about Window's implementations. I think it's worth it. |
Oh sorry, forgot to respond! I'd totally be in favor of implementing those traits for those types, it's precisely what we should be doing! As internal implementation details though, I was hoping to land this before giving it another go at the implementation level :) |
This pass performs a second pass of stabilization through the
std::sync
module, avoiding modules/types that are being handled in other PRs (e.g.
mutexes, rwlocks, condvars, and channels).
The following items are now stable
sync::atomic
sync::atomic::ATOMIC_BOOL_INIT
(wasINIT_ATOMIC_BOOL
)sync::atomic::ATOMIC_INT_INIT
(wasINIT_ATOMIC_INT
)sync::atomic::ATOMIC_UINT_INIT
(wasINIT_ATOMIC_UINT
)sync::Once
sync::ONCE_INIT
sync::Once::call_once
(wasdoit
)pthread_once(..)
call_once(..)
InitOnceExecuteOnce
sync::Barrier
sync::Barrier::new
sync::Barrier::wait
(now returns abool
)sync::Semaphore::new
sync::Semaphore::acquire
sync::Semaphore::release
The following items remain unstable
sync::SemaphoreGuard
sync::Semaphore::access
- it's unclear how this relates to the poisoningstory of mutexes.
sync::TaskPool
- the semantics of a failing task and whether a thread isre-attached to a thread pool are somewhat unclear, and the
utility of this type in
sync
is question with respect tothe jobs of other primitives. This type will likely become
stable or move out of the standard library over time.
sync::Future
- futures as-is have yet to be deeply re-evaluated with therecent core changes to Rust's synchronization story, and will
likely become stable in the future but are unstable until
that time comes.
[breaking-change]