From 64d0e19703ee06a7e92f4fdb816102b40ad2d743 Mon Sep 17 00:00:00 2001
From: Eliza Weisman <eliza@buoyant.io>
Date: Mon, 11 Apr 2022 14:50:36 -0700
Subject: [PATCH] core: fix scoped dispatchers clobbering the global default

## Motivation

PR #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 #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 #2050
---
 tracing-core/src/dispatcher.rs | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/tracing-core/src/dispatcher.rs b/tracing-core/src/dispatcher.rs
index efda4ebbb9..204c261e41 100644
--- a/tracing-core/src/dispatcher.rs
+++ b/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)
     }
 }