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

Tracking Issue for mutex_unpoison #96469

Closed
3 of 4 tasks
Tracked by #1
tmccombs opened this issue Apr 27, 2022 · 15 comments · Fixed by #119804
Closed
3 of 4 tasks
Tracked by #1

Tracking Issue for mutex_unpoison #96469

tmccombs opened this issue Apr 27, 2022 · 15 comments · Fixed by #119804
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@tmccombs
Copy link
Contributor

tmccombs commented Apr 27, 2022

Feature gate: #![feature(mutex_unpoison)]

This is a tracking issue for functions to clear the poisoned flag on Mutex and RwLock.

Public API

// std::sync

impl<T: ?Sized> Mutex<T> {
    pub fn clear_poison(&self);
}

impl<T: ?Sized> RwLock<T> {
    pub fn clear_poison(&self);
}

Steps / History

Unresolved Questions

  • None yet.
@tmccombs tmccombs added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 27, 2022
@GilShoshan94
Copy link
Contributor

GilShoshan94 commented Feb 28, 2023

Hi,

I read https://internals.rust-lang.org/t/unpoisoning-a-mutex/16521/3 (wich got automatically closed by the way) and the PR #96422.
It seems all good.

What is the next step to make it in the stable API ? Can I help somehow ?

@randommm
Copy link

Hi, sorry if flooding but are there any updates regarding this issue?

@tmccombs
Copy link
Contributor Author

I think the next step is one of the rust developers needs to start the final comment period. IDK what the criteria for doing so are.

@dtolnay
Copy link
Member

dtolnay commented Nov 12, 2023

@rust-lang/libs-api:
@rfcbot fcp merge

https://doc.rust-lang.org/nightly/std/sync/struct.Mutex.html#method.clear_poison
https://doc.rust-lang.org/nightly/std/sync/struct.RwLock.html#method.clear_poison

@rfcbot
Copy link

rfcbot commented Nov 12, 2023

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 12, 2023
@BurntSushi
Copy link
Member

I was wondering whether the idea of putting this method on the guard had been considered and found that @m-ou-se's commentary (and the ensuring comments) addressed it. SGTM!

randommm added a commit to randommm/rust-slackbot-llm that referenced this issue Nov 13, 2023
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Nov 13, 2023
@rfcbot
Copy link

rfcbot commented Nov 13, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 13, 2023
@U007D
Copy link

U007D commented Nov 21, 2023

Given the state change this method causes, I am curious about rationale behind using &self vs &mut self?

@bjorn3
Copy link
Member

bjorn3 commented Nov 21, 2023

If it took &mut self, it wouldn't be possible to unpoison a mutex shared between threads despite that being the main case where poisoning can happen.

@AnthonyMikh
Copy link
Contributor

AnthonyMikh commented Nov 23, 2023

I just want to add that probably before stabilizing this method we should implement a lint which catches invalid patterns of using these methods since it is very easy to misuse. Or, at the very least, include information about potential footgun in documentation for these methods.

@bjorn3
Copy link
Member

bjorn3 commented Nov 23, 2023

What kind of lint do you exactly want? What are the patterns in question that you are worried about?

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 23, 2023
@rfcbot
Copy link

rfcbot commented Nov 23, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@AnthonyMikh
Copy link
Contributor

What kind of lint do you exactly want? What are the patterns in question that you are worried about?

@bjorn3 exactly the possible error outlined by Mara Bos: calling clear_poison before acquiring a lock. The only way this usage is correct is when no other thread can act on mutex - either because they don't exist or because they are all blocked on something else. I believe that these situations are rare enough to flag such a pattern as at least suspicious.

@randommm
Copy link

If unpoison was a method of PoisonError or of MutexGuard instead of mutex, would it prevent such problems?

@tmccombs
Copy link
Contributor Author

That was discussed here:

#96422 (comment)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 14, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 11, 2024
…iper

Stabilize mutex_unpoison feature

Closes rust-lang#96469

`@rustbot` +T-libs-api
@bors bors closed this as completed in 1ed855d Jan 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 11, 2024
Rollup merge of rust-lang#119804 - tmccombs:stabilize-unpoison, r=cuviper

Stabilize mutex_unpoison feature

Closes rust-lang#96469

`@rustbot` +T-libs-api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants