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.

Sign up for GitHub

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

Explicitely drop the started var in examples #73074

Closed
wants to merge 1 commit into from
Closed

Explicitely drop the started var in examples #73074

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/libstd/sync/condvar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ impl WaitTimeoutResult {
/// break
/// }
/// }
/// // Leaving a condition variable's mutex held after we're no longer
/// // watching it will block and possibly deadlock notifier threads, so
Comment on lines +63 to +64
Copy link
Member

Choose a reason for hiding this comment

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

I think this is misleading. Just holding the mutex would not be sufficient to block a notifier thread. It just stops a notifier thread (or anything else) from acquiring the same mutex, which is the whole point of a mutex. If I understand correctly, the notify_one call does not block, so there is nothing about the behavior that would be specific to a notifier thread.

/// // we explicitely drop it.
Comment on lines +64 to +65
Copy link
Member

Choose a reason for hiding this comment

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

The salient point is to release the mutex as soon as possible, not to release it explicitly, so I also find this part misleading. Implicit release when it goes out of scope is just as good as long as it happens as soon as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then should I just make the comment something like It is sometimes necessary to manually drop the mutex to unlock it as soon as possible. If you need the ressource until the end of the scope, this is not necessary. and put it once in a new example on the mutex type itself ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think Mutex could use some example code that demonstrates a realistic use case requiring explicit drop.

/// std::mem::drop(started);
/// ```
#[stable(feature = "wait_timeout", since = "1.5.0")]
pub fn timed_out(&self) -> bool {
Expand Down Expand Up @@ -107,6 +111,10 @@ impl WaitTimeoutResult {
/// while !*started {
/// started = cvar.wait(started).unwrap();
/// }
/// // Leaving a condition variable's mutex held after we're no longer
/// // watching it will block and possibly deadlock notifier threads, so
/// // we explicitely drop it.
/// std::mem::drop(started);
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub struct Condvar {
Expand Down Expand Up @@ -191,6 +199,10 @@ impl Condvar {
/// while !*started {
/// started = cvar.wait(started).unwrap();
/// }
/// // Leaving a condition variable's mutex held after we're no longer
/// // watching it will block and possibly deadlock notifier threads, so
/// // we explicitely drop it.
/// std::mem::drop(started);
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn wait<'a, T>(&self, guard: MutexGuard<'a, T>) -> LockResult<MutexGuard<'a, T>> {
Expand Down Expand Up @@ -312,6 +324,10 @@ impl Condvar {
/// break
/// }
/// }
/// // Leaving a condition variable's mutex held after we're no longer
/// // watching it will block and possibly deadlock notifier threads, so
/// // we explicitely drop it.
/// std::mem::drop(started);
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_deprecated(since = "1.6.0", reason = "replaced by `std::sync::Condvar::wait_timeout`")]
Expand Down Expand Up @@ -385,6 +401,10 @@ impl Condvar {
/// break
/// }
/// }
/// // Leaving a condition variable's mutex held after we're no longer
/// // watching it will block and possibly deadlock notifier threads, so
/// // we explicitely drop it.
/// std::mem::drop(started);
/// ```
#[stable(feature = "wait_timeout", since = "1.5.0")]
pub fn wait_timeout<'a, T>(
Expand Down Expand Up @@ -513,6 +533,10 @@ impl Condvar {
/// while !*started {
/// started = cvar.wait(started).unwrap();
/// }
/// // Leaving a condition variable's mutex held after we're no longer
/// // watching it will block and possibly deadlock notifier threads, so
/// // we explicitely drop it.
/// std::mem::drop(started);
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn notify_one(&self) {
Expand Down Expand Up @@ -553,6 +577,10 @@ impl Condvar {
/// while !*started {
/// started = cvar.wait(started).unwrap();
/// }
/// // Leaving a condition variable's mutex held after we're no longer
/// // watching it will block and possibly deadlock notifier threads, so
/// // we explicitely drop it.
/// std::mem::drop(started);
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn notify_all(&self) {
Expand Down