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

std: Second pass stabilization of sync #20315

Merged
merged 1 commit into from
Jan 2, 2015
Merged

Conversation

alexcrichton
Copy link
Member

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]

@rust-highfive
Copy link
Collaborator

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned pcwalton Dec 29, 2014
@alexcrichton
Copy link
Member Author

cc @steveklabnik, the guide mentions futures and this may affect that

@alexcrichton alexcrichton force-pushed the std-sync branch 2 times, most recently from 3cebea3 to 5bd3650 Compare December 30, 2014 02:54
@aturon
Copy link
Member

aturon commented Dec 30, 2014

Overall this looks good, but I'm a bit uncomfortable stabilizing any of the methods of Semaphore until the panic/poison story is more clear. We might want acquire to yield a result, for example.

A smaller nit is the bool return type for wait on barriers; conventions demand it be an enum.

@alexcrichton
Copy link
Member Author

Rebased, semaphores are now #[unstable], and barriers now return a BarrierWaitResult with one is_leader method.

re-r? @aturon

@@ -218,9 +215,6 @@ pub struct UnixListener {
path: CString,
}

unsafe impl Send for UnixListener {}
unsafe impl Sync for UnixListener {}
Copy link
Contributor

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?

Copy link
Member Author

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.

@@ -117,9 +117,6 @@ pub struct UnixStream {
write_deadline: u64,
}

unsafe impl Send for UnixStream {}
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 I'm missing something but, why shouldn't UnixStream be Send and Sync ?

@flaper87 flaper87 assigned erickt and aturon and unassigned aturon and erickt Dec 30, 2014
@flaper87
Copy link
Contributor

@alexcrichton

Mmh why can't UnixStream be Send and Sync as well? It owns an Arc, which already implements these traits. The same applies for the other types in std::sys::unix

Am I missing something?

Also, it'd be nice to use this change to make both, Unix and windows, implementations consistent.

@alexcrichton
Copy link
Member Author

UnixStream is automatically inferred to be Send and Sync, it doesn't need the extra unsafe impl blocks (or so I am led to believe). I also believe that the same applies for AcceptorInner.

The windows implementations need manual unsafe impl blocks due to containing libc::HANDLE which is just a raw pointer which is itself not Send or Sync.

@flaper87
Copy link
Contributor

@alexcrichton yup, it's inferred to be Send and Sync but I preferred there to be explicit implementations. However, after putting some extra thought on this, I think it's better not to have explicit implementations when they are no needed, this will protect us from mistakenly breaking types guarantees without noticing. That is to say, if a change to UnixStream makes it NoSend, we'll get errors.

re-windows: fair enough. Though, I wonder if we should implement Send and Sync for Inner and Event instead of implementing them for UnixStream, AcceptorState etc. That'd remove some of the explicit implementations of these traits. It'll also make the unix and windows implementations more consistent and finally, it'll prevent us from making some dumb mistakes.

Otherwise, LGTM.

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]
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 2, 2015
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
@flaper87
Copy link
Contributor

flaper87 commented Jan 2, 2015

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

@alexcrichton
Copy link
Member Author

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 :)

@bors bors merged commit f3a7ec7 into rust-lang:master Jan 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants