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

subscriber: always ignore lock poisoning #1063

Merged
merged 4 commits into from
Oct 24, 2020

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Oct 22, 2020

Motivation

tracing-subscriber can conditionally use parking_lot's locks rather
than std::sync's locks, when a feature flag is enabled. Because
parking_lot's locks do not poison on panics, while std::sync's locks
do, the APIs differ slightly. Currently, we handle this by wrapping the
parking_lot RwLock in a newtype that returns Results, like
std::sync::RwLock does, but always returns Ok. Then, potentially
poisoned locks are handled at the callsite.

However, this is unnecessary (and potentially incorrect). Because locks
may or may not be poisoned depending on the feature flag,
tracing-subscriber can't rely on poisoning for correctness. Doing so
would mean potentially different behavior between std::sync and
parking_lot. Currently, because a number of tracing-subscriber
functions are called in Drop impls, we don't panic when a lock is
poisoned, to avoid double panics. Most cases where a lock might be
poisoned are handled by returning early with a default value. However,
with parking_lot enabled, this will never happen, and it adds
complexity at the callsite.

Solution

This branch changes our approach to wrap std::sync's locks and use
PoisonError::into_inner to ignore poisoning. This results in more
consistent behavior between the parking_lot feature and std, and
also probably improves performance a bit with parking_lot --- we don't
have to have extra code for handling an Err case that will never
happen (although rustc might be smart enough to optimize it away). Also,
the callsites are simpler since they never have to handle poisoning, and
we can't accidentally unwrap in a Drop impl.

I also removed the janky user-space thread-local implementation used by
CurrentSpan, and changed it to just use the thread-local crate. We
already depend on that crate for the Registry, and my DIY
implementation has some performance issues. So, I removed it.

Depends on #1062

`tracing-subscriber` can conditionally use `parking_lot`'s locks rather
than `std::sync`'s locks, when a feature flag is enabled. Because
`parking_lot`'s locks do not poison on panics, while `std::sync`'s locks
do, the APIs differ slightly. Currently, we handle this by wrapping the
`parking_lot` `RwLock` in a newtype that returns `Result`s, like
`std::sync::RwLock` does, but always returns `Ok`. Then, potentially
poisoned locks are handled at the callsite.

However, this is unnecessary (and potentially incorrect). Because locks
may or may not be poisoned depending on the feature flag,
`tracing-subscriber` can't rely on poisoning for correctness. Doing so
would mean potentially different behavior between `std::sync` and
`parking_lot`. Currently, because a number of `tracing-subscriber`
functions are called in `Drop` impls, we don't panic when a lock is
poisoned, to avoid double panics. Most cases where a lock might be
poisoned are handled by returning early with a default value. However,
with `parking_lot` enabled, this will never happen, and it adds
complexity at the callsite.

This branch changes our approach to wrap `std::sync`'s locks and use
`PoisonError::into_inner` to ignore poisoning. This results in more
consistent behavior between the `parking_lot` feature and `std`, and
also probably improves performance a bit with `parking_lot` --- we don't
have to have extra code for handling an `Err` case that will never
happen (although rustc might be smart enough to optimize it away). Also,
the callsites are simpler since they never have to handle poisoning, and
we can't accidentally `unwrap` in a `Drop` impl.

I also removed the janky user-space thread-local implementation used by
`CurrentSpan`, and changed it to just use the `thread-local` crate. We
already depend on that crate for the `Registry`, and my DIY
implementation has some performance issues. So, I removed it.

Depends on #1062
@hawkw hawkw requested a review from davidbarsky October 22, 2020 20:31
@hawkw hawkw requested a review from a team as a code owner October 22, 2020 20:31
@hawkw hawkw merged commit 8274114 into eliza/update-slab Oct 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant