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

core: Mark DefaultGuard as !Send #2488

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bgw
Copy link

@bgw bgw commented Feb 26, 2023

Motivation

#2482: Dropping DefaultGuard in another thread doesn't cause it to get cleaned up properly

The Drop impl of DefaultGuard modifies the CURRENT_STATE thread local, assuming it to be the same thread local it was constructed with.

However, because DefaultGuard is Send, that may be a different thread local.

Solution

As discussed in #2482, we should mark DefaultGuard as !Send. This is a breaking API change.

This PR is modeled after #1001, which does the same thing for Entered. One notable change is that I use a different trick for marking the struct as !Send that doesn't involve unsafe.

Entered currently uses:

tracing/tracing/src/span.rs

Lines 1581 to 1591 in a0126b2

struct PhantomNotSend {
ghost: PhantomData<*mut ()>,
}
#[allow(non_upper_case_globals)]
const PhantomNotSend: PhantomNotSend = PhantomNotSend { ghost: PhantomData };
/// # Safety
///
/// Trivially safe, as `PhantomNotSend` doesn't have any API.
unsafe impl Sync for PhantomNotSend {}

My PR proposes the following trick instead:

/// A trait that implements `Sync`, but not `Send`. Used with PhantomData, this
/// can mark a struct as `!Send`.
#[cfg(feature = "std")]
trait NotSend: Sync {}

pub struct DefaultGuard(
    // ...
    PhantomData<dyn NotSend>,
);

If accepted:

  • I can follow up with a documentation change for 0.1.x that explains the thread safety issue.
  • I can follow up with a change to tracing/src/span.rs that removes the use of unsafe to implement !Send.

The `Drop` impl of `DefaultGuard` modifies the `CURRENT_STATE` thread
local, assuming it to be the same thread local it was constructed with.

However, because `DefaultGuard` is `Send`, that may be a different
thread local.

Therefore, we should mark `DefaultGuard` as `!Send`.
@bgw bgw requested review from hawkw and carllerche as code owners February 26, 2023 20:39
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