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

Jump to bottom

Example about explicit mutex dropping #73104

Merged
merged 7 commits into from
Jun 15, 2020
Merged

Example about explicit mutex dropping #73104

merged 7 commits into from
Jun 15, 2020

Conversation

poliorcetics
Copy link
Contributor

Fixes #67457.

Following the remarks made in #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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 7, 2020
LeSeulArtichaut
LeSeulArtichaut reviewed Jun 8, 2020
View reviewed changes
src/libstd/sync/mutex.rs Outdated Show resolved Hide resolved
src/libstd/sync/mutex.rs Outdated Show resolved Hide resolved
@poliorcetics @LeSeulArtichaut
Simply use drop instead of std::mem::drop
fdef1a5 
Co-authored-by: LeSeulArtichaut <leseulartichaut@gmail.com>
nagisa
nagisa reviewed Jun 9, 2020
View reviewed changes
Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

NB: Review comments I made are entirely a suggestion, they in no way mean I reject (were comments not resolved) or approve (once/if the comments are resolved) the PR.

src/libstd/sync/mutex.rs Show resolved Hide resolved
src/libstd/sync/mutex.rs Show resolved Hide resolved
@poliorcetics
Copy link
Contributor Author

@nagisa I removed a lot of comments. Aside from the ones about the dummy work, there is only the big comment about deadlocking in main, which explains pretty much everything (I hope).

Do not hesitate to tell me if you see something else I can improve !

@poliorcetics
Copy link
Contributor Author

@rustbot modify labels: C-enhancement, T-doc, T-libs

@rustbot rustbot added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 11, 2020
dtolnay
dtolnay requested changes Jun 13, 2020
View reviewed changes
src/libstd/sync/mutex.rs Outdated Show resolved Hide resolved
src/libstd/sync/mutex.rs Outdated Show resolved Hide resolved
src/libstd/sync/mutex.rs Outdated Show resolved Hide resolved
src/libstd/sync/mutex.rs Outdated Show resolved Hide resolved
src/libstd/sync/mutex.rs Outdated Show resolved Hide resolved
src/libstd/sync/mutex.rs Outdated Show resolved Hide resolved
poliorcetics and others added 2 commits June 13, 2020 18:41
@poliorcetics @dtolnay
Apply suggestions from code review
f747073 
Co-authored-by: David Tolnay <dtolnay@gmail.com>
@poliorcetics
Clarify the scope-related explanation
34b3ff0 
Based on the review made by dtolnay.
@poliorcetics poliorcetics requested a review from dtolnay June 13, 2020 16:44
dtolnay
dtolnay approved these changes Jun 13, 2020
View reviewed changes
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.

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Jun 13, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jun 13, 2020

📌 Commit c010e71 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2020
@poliorcetics
Copy link
Contributor Author

I'm sorry you had to do a formatting commit, running rustfmt did nothing to the file locally for me.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 14, 2020
@Dylan-DPC
Rollup merge of rust-lang#73104 - poliorcetics:explicit-mutex-drop-ex…
e1d05a9 
…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.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Jun 14, 2020
Closed
@RalfJung
Copy link
Member

@bors rollup

RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 15, 2020
@RalfJung
Rollup merge of rust-lang#73104 - poliorcetics:explicit-mutex-drop-ex…
13d5c27 
…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.
This was referenced Jun 15, 2020
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 15, 2020
@bors
Auto merge of rust-lang#73369 - RalfJung:rollup-hl8g9zf, r=RalfJung
ff4a253 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#72707 (Use min_specialization in the remaining rustc crates)
 - rust-lang#72740 (On recursive ADT, provide indirection structured suggestion)
 - rust-lang#72879 (Miri: avoid tracking current location three times)
 - rust-lang#72938 (Stabilize Option::zip)
 - rust-lang#73086 (Rename "cyclone" to "apple-a7" per changes in upstream LLVM)
 - rust-lang#73104 (Example about explicit mutex dropping)
 - rust-lang#73139 (Add methods to go from a nul-terminated Vec<u8> to a CString)
 - rust-lang#73296 (Remove vestigial CI job msvc-aux.)
 - rust-lang#73304 (Revert heterogeneous SocketAddr PartialEq impls)
 - rust-lang#73331 (extend network support for HermitCore)

Failed merges:

r? @ghost
@bors bors merged commit 7c8b941 into rust-lang:master Jun 15, 2020
@poliorcetics poliorcetics deleted the explicit-mutex-drop-example branch June 16, 2020 21:18
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeSeulArtichaut LeSeulArtichaut LeSeulArtichaut left review comments

@nagisa nagisa nagisa left review comments

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.46
Development

Successfully merging this pull request may close these issues.

Example CondVar code should explicitly drop the MutexGuard after it's done with it
9 participants
@poliorcetics @dtolnay @bors @RalfJung @nagisa @LeSeulArtichaut @cuviper @rust-highfive @rustbot