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

chore: handle std Mutex poisoning in a shim #2872

Merged
merged 4 commits into from Sep 25, 2020

Conversation

zaharidichev
Copy link
Contributor

Motivation

As tokio does not rely on poisoning, we can
avoid always unwrapping when locking by handling
the PoisonError in the Mutex shim.

Solution

Provide a wrapper around std::sync::Mutex that uses
PoisonError::into_inner to ignore the poisoning aspects
of the std api.

Fix #2867

Signed-off-by: Zahari Dichev zaharidichev@gmail.com

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup. M-sync Module: tokio/sync labels Sep 24, 2020
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

LGTM thanks 👍

@carllerche
Copy link
Member

Looks like there is a conflict.

As tokio does not rely on poisoning, we can
avoid always unwrapping when locking by handling
the `PoisonError` in the Mutex shim.

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
@zaharidichev
Copy link
Contributor Author

resolved conflicts

Comment on lines +17 to +20
#[inline]
pub(crate) fn lock(&self) -> MutexGuard<'_, T> {
self.0.lock().unwrap()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't really seem correct? Shouldn't it use into_inner? Or maybe it's good to panic on poisoning in loom tests?

Copy link
Member

Choose a reason for hiding this comment

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

Poisoning in loom is good because we should never hit it.

@carllerche carllerche merged commit 4446606 into tokio-rs:master Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup. M-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: update internal Mutex shim
3 participants