Skip to content

Commit

Permalink
core: don't use NoSubscriber as local placeholder (tokio-rs#2001)
Browse files Browse the repository at this point in the history
## Motivation

Currently, it is not actually possible to use `set_default(NoSubscriber)`
or similar to temporarily disable the global default subscriber (see
tokio-rs#1999).

This is because `NoSubscriber` is currently used as a placeholder value
when the thread-local cell that stores the current scoped default
subscriber is initialized. Therefore, we currently check if the current
scoped subscriber is `NoSubscriber`, and if it is, we fall back to
returning the global default instead.

This was fine, _when `NoSubscriber` was a private internal type only_.
However, PR tokio-rs#1549 makes `NoSubscriber` into a public API type. When users
can publicly construct `NoSubscriber` instances, it makes sense to want
to be able to use `NoSubscriber` to disable the current subscriber. This
is not possible when there is a global default set, because the local
default being `NoSubscriber` will cause the global default to be
returned.

## Solution

This branch changes the thread-local cell to store an `Option<Dispatch>`
instead, and use the `None` case to indicate no local default is set.
This way, when the local default is explicitly set to `NoSubscriber`, we
will return `NoSubscriber` rather than falling back.

This may also be a slight performance improvement, because we now check
if there's no global default by checking if the `Option` is `None`,
rather than downcasting it to a `NoSubscriber`.

I've also added a test reproducing tokio-rs#1999.

Fixes tokio-rs#1999

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
  • Loading branch information
hawkw authored and kaffarell committed May 22, 2024
1 parent 0bbf521 commit 19f31d8
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 15 deletions.
32 changes: 17 additions & 15 deletions tracing-core/src/dispatcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ pub struct Dispatch {
#[cfg(feature = "std")]
thread_local! {
static CURRENT_STATE: State = State {
default: RefCell::new(Dispatch::none()),
default: RefCell::new(None),
can_enter: Cell::new(true),
};
}
Expand All @@ -178,7 +178,7 @@ static mut GLOBAL_DISPATCH: Option<Dispatch> = None;
#[cfg(feature = "std")]
struct State {
/// This thread's current default dispatcher.
default: RefCell<Dispatch>,
default: RefCell<Option<Dispatch>>,
/// Whether or not we can currently begin dispatching a trace event.
///
/// This is set to `false` when functions such as `enter`, `exit`, `event`,
Expand Down Expand Up @@ -641,7 +641,9 @@ impl Default for Dispatch {

impl fmt::Debug for Dispatch {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.pad("Dispatch(...)")
f.debug_tuple("Dispatch")
.field(&format_args!("{:p}", self.subscriber))
.finish()
}
}

Expand Down Expand Up @@ -682,7 +684,13 @@ impl State {
let prior = CURRENT_STATE
.try_with(|state| {
state.can_enter.set(true);
state.default.replace(new_dispatch)
state
.default
.replace(Some(new_dispatch))
// if the scoped default was not set on this thread, set the
// `prior` default to the global default to populate the
// scoped default when unsetting *this* default
.unwrap_or_else(|| get_global().cloned().unwrap_or_else(Dispatch::none))
})
.ok();
EXISTS.store(true, Ordering::Release);
Expand All @@ -705,16 +713,10 @@ impl State {
impl<'a> Entered<'a> {
#[inline]
fn current(&self) -> RefMut<'a, Dispatch> {
let mut default = self.0.default.borrow_mut();

if default.is::<NoSubscriber>() {
if let Some(global) = get_global() {
// don't redo this call on the next check
*default = global.clone();
}
}

default
let default = self.0.default.borrow_mut();
RefMut::map(default, |default| {
default.get_or_insert_with(|| get_global().cloned().unwrap_or_else(Dispatch::none))
})
}
}

Expand All @@ -738,7 +740,7 @@ impl Drop for DefaultGuard {
// lead to the drop of a subscriber which, in the process,
// could then also attempt to access the same thread local
// state -- causing a clash.
let prev = CURRENT_STATE.try_with(|state| state.default.replace(dispatch));
let prev = CURRENT_STATE.try_with(|state| state.default.replace(Some(dispatch)));
drop(prev)
}
}
Expand Down
17 changes: 17 additions & 0 deletions tracing/tests/no_subscriber.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#![cfg(feature = "std")]

use tracing::subscriber::{self, NoSubscriber};

mod support;

#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
#[test]
fn no_subscriber_disables_global() {
// Reproduces https://github.com/tokio-rs/tracing/issues/1999
let (subscriber, handle) = support::subscriber::mock().done().run_with_handle();
subscriber::set_global_default(subscriber).expect("setting global default must succeed");
subscriber::with_default(NoSubscriber::default(), || {
tracing::info!("this should not be recorded");
});
handle.assert_finished();
}

0 comments on commit 19f31d8

Please sign in to comment.