-
Notifications
You must be signed in to change notification settings - Fork 727
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: fix scoped dispatchers clobbering the global default #2065
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## 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
hawkw
added
kind/bug
Something isn't working
crate/core
Related to the `tracing-core` crate
labels
Apr 11, 2022
davidbarsky
approved these changes
Apr 12, 2022
hawkw
added a commit
that referenced
this pull request
Apr 12, 2022
# 0.1.25 (April 12, 2022) This release adds additional `Value` implementations for `std::error::Error` trait objects with auto trait bounds (`Send` and `Sync`), as Rust will not auto-coerce trait objects. Additionally, it fixes a bug when setting scoped dispatchers that was introduced in the previous release ([v0.1.24]). ### Added - `Value` implementations for `dyn Error + Send + 'static`, `dyn Error + Send + Sync + 'static`, `dyn Error + Sync + 'static` ([#2066]) ### Fixed - Failure to use the global default dispatcher if a thread has set a scoped default prior to setting the global default, and unset the scoped default after setting the global default ([#2065]) Thanks to @lilyball for contributing to this release! [v0.1.24]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.24 [#2066]: #2066 [#2065]: #2065
hawkw
added a commit
that referenced
this pull request
Apr 12, 2022
# 0.1.25 (April 12, 2022) This release adds additional `Value` implementations for `std::error::Error` trait objects with auto trait bounds (`Send` and `Sync`), as Rust will not auto-coerce trait objects. Additionally, it fixes a bug when setting scoped dispatchers that was introduced in the previous release ([v0.1.24]). ### Added - `Value` implementations for `dyn Error + Send + 'static`, `dyn Error + Send + Sync + 'static`, `dyn Error + Sync + 'static` ([#2066]) ### Fixed - Failure to use the global default dispatcher if a thread has set a scoped default prior to setting the global default, and unset the scoped default after setting the global default ([#2065]) Thanks to @lilyball for contributing to this release! [v0.1.24]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.24 [#2066]: #2066 [#2065]: #2065
hawkw
added a commit
that referenced
this pull request
Apr 14, 2022
# 0.1.34 (April 14, 2022) This release includes bug fixes for the "log" support feature and for the use of both scoped and global default dispatchers in the same program. ### Fixed - Failure to use the global default dispatcher when a thread sets a local default dispatcher before the global default is set ([#2065]) - **log**: Compilation errors due to `async` block/fn futures becoming `!Send` when the "log" feature flag is enabled ([#2073]) - Broken links in documentation ([#2068]) Thanks to @ben0x539 for contributing to this release! [#2065]: #2065 [#2073]: #2073 [#2068]: #2068
hawkw
added a commit
that referenced
this pull request
Apr 14, 2022
# 0.1.34 (April 14, 2022) This release includes bug fixes for the "log" support feature and for the use of both scoped and global default dispatchers in the same program. ### Fixed - Failure to use the global default dispatcher when a thread sets a local default dispatcher before the global default is set ([#2065]) - **log**: Compilation errors due to `async` block/fn futures becoming `!Send` when the "log" feature flag is enabled ([#2073]) - Broken links in documentation ([#2068]) Thanks to @ben0x539 for contributing to this release! [#2065]: #2065 [#2073]: #2073 [#2068]: #2068
hawkw
added a commit
that referenced
this pull request
Apr 14, 2022
# 0.1.34 (April 14, 2022) This release includes bug fixes for the "log" support feature and for the use of both scoped and global default dispatchers in the same program. ### Fixed - Failure to use the global default dispatcher when a thread sets a local default dispatcher before the global default is set ([#2065]) - **log**: Compilation errors due to `async` block/fn futures becoming `!Send` when the "log" feature flag is enabled ([#2073]) - Broken links in documentation ([#2068]) Thanks to @ben0x539 for contributing to this release! [#2065]: #2065 [#2073]: #2073 [#2068]: #2068
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
May 22, 2024
…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
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
May 22, 2024
# 0.1.25 (April 12, 2022) This release adds additional `Value` implementations for `std::error::Error` trait objects with auto trait bounds (`Send` and `Sync`), as Rust will not auto-coerce trait objects. Additionally, it fixes a bug when setting scoped dispatchers that was introduced in the previous release ([v0.1.24]). ### Added - `Value` implementations for `dyn Error + Send + 'static`, `dyn Error + Send + Sync + 'static`, `dyn Error + Sync + 'static` ([tokio-rs#2066]) ### Fixed - Failure to use the global default dispatcher if a thread has set a scoped default prior to setting the global default, and unset the scoped default after setting the global default ([tokio-rs#2065]) Thanks to @lilyball for contributing to this release! [v0.1.24]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.24 [tokio-rs#2066]: tokio-rs#2066 [tokio-rs#2065]: tokio-rs#2065
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
May 22, 2024
# 0.1.34 (April 14, 2022) This release includes bug fixes for the "log" support feature and for the use of both scoped and global default dispatchers in the same program. ### Fixed - Failure to use the global default dispatcher when a thread sets a local default dispatcher before the global default is set ([tokio-rs#2065]) - **log**: Compilation errors due to `async` block/fn futures becoming `!Send` when the "log" feature flag is enabled ([tokio-rs#2073]) - Broken links in documentation ([tokio-rs#2068]) Thanks to @ben0x539 for contributing to this release! [tokio-rs#2065]: tokio-rs#2065 [tokio-rs#2073]: tokio-rs#2073 [tokio-rs#2068]: tokio-rs#2068
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
. Thismeans dropping a
DefaultGuard
that was created before any othersubscriber was set as default will reset that thread's default to
NoSubscriber
. Because #2001 changed the dispatcher module to stopusing
NoSubscriber
as a placeholder for "use the global default if oneexists", 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 nodefault has been set. Instead of populating the "previous" dispatcher
with
NoSubscriber
, it instead leaves theDefaultGuard
with aNone
.When the
DefaultGuard
is dropped, if the subscriber isNone
, it willjust clear the thread-local cell, rather than setting it to
NoSubscriber
. This way, the next time the cell is accessed, we willcheck 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 thanmaster
, because the issue doesnot 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