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

Explicitely drop the started var in examples #73074

Conversation

poliorcetics
Copy link
Contributor

Fixes #67457.

I added the explicit drop with an explanation, it is enough ?

@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2020
Comment on lines +63 to +64
/// // Leaving a condition variable's mutex held after we're no longer
/// // watching it will block and possibly deadlock notifier threads, so
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.

Comment on lines +64 to +65
/// // watching it will block and possibly deadlock notifier threads, so
/// // we explicitely drop it.
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.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Per #73074 (comment) I'll close this and we can follow up in a separate PR adding that example to Mutex.

@dtolnay dtolnay closed this Jun 7, 2020
@poliorcetics poliorcetics deleted the condvar-example-drop-mutexguard branch June 7, 2020 20:37
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 14, 2020
…ample, r=dtolnay

Example about explicit mutex dropping

Fixes rust-lang#67457.

Following the remarks made in rust-lang#73074, I added an example on the main `Mutex` type, with a situation where there is mutable data and a computation result.

In my testing it is effectively needed to explicitly drop the lock, else it deadlocks.

r? @dtolnay because you were the one to review the previous PR.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 15, 2020
…ample, r=dtolnay

Example about explicit mutex dropping

Fixes rust-lang#67457.

Following the remarks made in rust-lang#73074, I added an example on the main `Mutex` type, with a situation where there is mutable data and a computation result.

In my testing it is effectively needed to explicitly drop the lock, else it deadlocks.

r? @dtolnay because you were the one to review the previous PR.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 15, 2020
…ample, r=dtolnay

Example about explicit mutex dropping

Fixes rust-lang#67457.

Following the remarks made in rust-lang#73074, I added an example on the main `Mutex` type, with a situation where there is mutable data and a computation result.

In my testing it is effectively needed to explicitly drop the lock, else it deadlocks.

r? @dtolnay because you were the one to review the previous PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Example CondVar code should explicitly drop the MutexGuard after it's done with it
3 participants