core: don't use NoCollector as local placeholder #2001

hawkw · 2022-03-18T18:55:39Z

Motivation

Currently, it is not actually possible to use set_default(NoCollector)
or similar to temporarily disable the global default collector (see
#1999).

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

This was fine, when NoCollector was a private internal type only.
However, PR #1549 makes NoCollector into a public API type. When users
can publicly construct NoCollector instances, it makes sense to want
to be able to use NoCollector to disable the current collector. This
is not possible when there is a global default set, because the local
default being NoCollector 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 NoCollector, we
will return NoCollector 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 NoCollector.

I've also added a test reproducing #1999.

Fixes #1999

Signed-off-by: Eliza Weisman <[email protected]>

hrxi · 2022-03-18T19:17:57Z

What's the policy on backporting fixes? Should I backport this PR myself if I want to see it on the 0.1 branch?

davidbarsky · 2022-03-18T19:24:37Z

@hrxi I'll backport it to v0.1.0; don't worry about it!

## Motivation Currently, it is not actually possible to use `set_default(NoSubscriber)` or similar to temporarily disable the global default subscriber (see #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 #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 #1999. Fixes #1999 Signed-off-by: Eliza Weisman <[email protected]>

# 0.1.24 (April 1, 2022) This release fixes a bug where setting `NoSubscriber` as the local default would not locally disable the current global default subscriber. ### Fixed - Setting `NoSubscriber` as the local default now correctly disables the global default subscriber ([#2001]) - Fixed compilation warnings with the "std" feature disabled ([#2022]) ### Changed - Removed unnecessary use of `write!` and `format_args!` macros ([#1988]) [#1988]: #1988 [#2001]: #2001 [#2022]: #2022

## 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

## 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 <[email protected]>

# 0.1.24 (April 1, 2022) This release fixes a bug where setting `NoSubscriber` as the local default would not locally disable the current global default subscriber. ### Fixed - Setting `NoSubscriber` as the local default now correctly disables the global default subscriber ([tokio-rs#2001]) - Fixed compilation warnings with the "std" feature disabled ([tokio-rs#2022]) ### Changed - Removed unnecessary use of `write!` and `format_args!` macros ([tokio-rs#1988]) [tokio-rs#1988]: tokio-rs#1988 [tokio-rs#2001]: tokio-rs#2001 [tokio-rs#2022]: tokio-rs#2022

…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

hawkw added 3 commits March 18, 2022 10:57

add test reproducing #1999

fe18d68

Signed-off-by: Eliza Weisman <[email protected]>

core: give Dispatch a nicer Debug impl

963c5e9

Signed-off-by: Eliza Weisman <[email protected]>

core: don't use NoCollector as local placeholder

4078bfd

Signed-off-by: Eliza Weisman <[email protected]>

hawkw requested review from carllerche, davidbarsky and a team as code owners March 18, 2022 18:55

davidbarsky approved these changes Mar 18, 2022

View reviewed changes

hawkw merged commit 4adc0a3 into master Mar 18, 2022

hawkw deleted the eliza/no-collector-fix branch March 18, 2022 19:33

hawkw mentioned this pull request Mar 18, 2022

backport recent changes to v0.1.x #2002

Merged

This was referenced Mar 24, 2022

Dispatcher::none() and NoSubscriber have no effect when set per-thread #2016

Closed

Don't trace tasks spawned through the console server tokio-rs/console#314

Merged

hawkw mentioned this pull request Apr 1, 2022

core: prepare to release 0.1.24 #2042

Merged

hawkw mentioned this pull request Apr 11, 2022

Events missing using tracing-core 0.1.24 but works using 0.1.23 #2050

Closed

hawkw mentioned this pull request Apr 11, 2022

core: fix scoped dispatchers clobbering the global default #2065

Merged

davidbarsky mentioned this pull request Sep 26, 2023

chore: backport roughly a year's worth of changes #2728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: don't use NoCollector as local placeholder #2001

core: don't use NoCollector as local placeholder #2001

hawkw commented Mar 18, 2022

hrxi commented Mar 18, 2022

davidbarsky commented Mar 18, 2022

core: don't use NoCollector as local placeholder #2001

core: don't use NoCollector as local placeholder #2001

Conversation

hawkw commented Mar 18, 2022

Motivation

Solution

hrxi commented Mar 18, 2022

davidbarsky commented Mar 18, 2022