core: fix scoped dispatchers clobbering the global default (tokio-rs#…

…2065)

## Motivation

PR tokio-rs#2001 introduced --- or rather, _uncovered_ --- a bug which occurs
when a global default subscriber is set *after* a scoped default has
been set.

When the scoped default guard is dropped, it resets the
thread-local default cell to whatever subscriber was the default when
the scoped default was set. This allows nesting scoped default contexts.
However, when there was *no* default subscriber when the `DefaultGuard`
was created, it sets the "previous" subscriber as `NoSubscriber`. This
means dropping a `DefaultGuard` that was created before any other
subscriber was set as default will reset that thread's default to
`NoSubscriber`. Because tokio-rs#2001 changed the dispatcher module to stop
using `NoSubscriber` as a placeholder for "use the global default if one
exists", this means that the global default is permanently clobbered on
the thread that set the scoped default prior to setting the global one.

## Solution

This PR changes the behavior when creating a `DefaultGuard` when no
default has been set. Instead of populating the "previous" dispatcher
with `NoSubscriber`, it instead leaves the `DefaultGuard` with a `None`.
When the `DefaultGuard` is dropped, if the subscriber is `None`, it will
just clear the thread-local cell, rather than setting it to
`NoSubscriber`. This way, the next time the cell is accessed, we will
check if a global default exists to populate the thread-local, and
everything works correctly. As a side benefit, this also makes the code
a bit simpler!

I've also added a test reproducing the bug.

This PR is against `v0.1.x` rather than `master`, because the issue does
not exist on `master` due to other implementation differences in v0.2.
We may want to forward-port the test to guard against future
regressions, though.

Fixes tokio-rs#2050

tracing-core/src/dispatcher.rs

@@ -684,15 +684,10 @@ impl State {
         let prior = CURRENT_STATE
             .try_with(|state| {
                 state.can_enter.set(true);
-                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))
+                state.default.replace(Some(new_dispatch))
             })
-            .ok();
+            .ok()
+            .flatten();
         EXISTS.store(true, Ordering::Release);
         DefaultGuard(prior)
     }
@@ -734,15 +729,13 @@ impl<'a> Drop for Entered<'a> {
 impl Drop for DefaultGuard {
     #[inline]
     fn drop(&mut self) {
-        if let Some(dispatch) = self.0.take() {
-            // Replace the dispatcher and then drop the old one outside
-            // of the thread-local context. Dropping the dispatch may
-            // 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(Some(dispatch)));
-            drop(prev)
-        }
+        // Replace the dispatcher and then drop the old one outside
+        // of the thread-local context. Dropping the dispatch may
+        // 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(self.0.take()));
+        drop(prev)
     }
 }
 

tracing/tests/scoped_clobbers_default.rs

@@ -0,0 +1,35 @@
+#![cfg(feature = "std")]
+use tracing_mock::*;
+
+#[test]
+fn scoped_clobbers_global() {
+    // Reproduces https://github.com/tokio-rs/tracing/issues/2050
+
+    let (scoped, scoped_handle) = subscriber::mock()
+        .event(event::msg("before global"))
+        .event(event::msg("before drop"))
+        .done()
+        .run_with_handle();
+
+    let (global, global_handle) = subscriber::mock()
+        .event(event::msg("after drop"))
+        .done()
+        .run_with_handle();
+
+    // Set a scoped default subscriber, returning a guard.
+    let guard = tracing::subscriber::set_default(scoped);
+    tracing::info!("before global");
+
+    // Now, set the global default.
+    tracing::subscriber::set_global_default(global)
+        .expect("global default should not already be set");
+    // This event should still be collected by the scoped default.
+    tracing::info!("before drop");
+
+    // Drop the guard. Now, the global default subscriber should be used.
+    drop(guard);
+    tracing::info!("after drop");
+
+    scoped_handle.assert_finished();
+    global_handle.assert_finished();
+}