-
Notifications
You must be signed in to change notification settings - Fork 729
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
tracing: make Entered
!Send
#1001
Conversation
The `Entered` guard returned by `Span::enter` represents entering and exiting a span _on the current thread_. Calling `Span::enter` enters the span, returning an `Entered`, and dropping the `Entered` ensures that the span is exited. This ensures that all spans, once entered, are eventually exited. However, `Entered` has an auto-impl of `Send`, because it doesn't contain any `!Send` types. This means that the `Entered` guard _could_ be sent to another thread and dropped. This would cause the original thread to never exit the span,. and the thread to which the `Entered` guard was sent would exit a span that it never observed an enter for. This is incorrect. This PR adds a `*mut ()` field to `Entered` so that it no longer implements `Send`. There's now a manual `Sync` impl so structs holding an `Entered` can still be `Sync`. Fixes #698 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@@ -379,6 +380,7 @@ pub(crate) struct Inner { | |||
#[must_use = "once a span has been entered, it should be exited"] | |||
pub struct Entered<'a> { | |||
span: &'a Span, | |||
_not_send: PhantomData<*mut ()>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_not_send: PhantomData<*mut ()>, | |
_not_send_but_sync: PhantomData<std::sync::MutexGuard<'static, ()>>, |
It's a bit weird, but -1 unsafe, because you don't need to unsafe impl Sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh that's clever! thanks!
unfortunately we can't do exactly what you've suggested, because this needs to compile with the std
feature disabled, but I can do something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually unsafe? We are not dereferencing this type, thus doesn't require unsafe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LucioFranco the unsafe was necessary for manually implementing Sync
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and, it turns out this approach doesn't actually work in this specific case. Because tracing
has conditional support for no_std
, we can't use the std::sync
version of MutexGuard
. I thought we could use the MutexGuard
from the spinlock implementation that's used as a fallback on no-std platforms, but I'd forgotten that this is defined in tracing-core
, not tracing
, and isn't publicly visible. I'll probably go back to the previous approach of just accepting the additional unsafe
keyword necessary to manually impl Sync
.
thanks @MikailBag for the suggestion! Signed-off-by: Eliza Weisman <eliza@buoyant.io>
…o eliza/0.2-api-fixes
This reverts commit 81e9e5b. . Ut turns out this approach doesn't actually work in this specific case. Because `tracing` has conditional support for `no_std`, we can't use the `std::sync` version of `MutexGuard`. I thought we could use the `MutexGuard` from the spinlock implementation that's used as a fallback on no-std platforms, but I'd forgotten that this is defined in `tracing-core`, not `tracing`, and isn't publicly visible. I'll probably go back to the previous approach of just accepting the additional `unsafe` keyword necessary to manually impl `Sync`.
* upstream/master: subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) docs: switch to intra-doc links in tracing-core (tokio-rs#1010) tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007) core: add intrusive linked list for callsite registry (tokio-rs#988) serde: allow tracing-serde to work on no_std. (tokio-rs#960) tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003) tracing: make `Entered` `!Send` (tokio-rs#1001) chore: fix nightly clippy warnings (tokio-rs#991) chore: bump all crate versions (tokio-rs#998) macros: fix the `tracing-macros` crate not compiling (tokio-rs#1000) tracing: prepare to release 0.1.21 (tokio-rs#997) core: prepare to release 0.1.17 (tokio-rs#996) subscriber: make `PartialOrd` & `Ord` impls more correct (tokio-rs#995) core, tracing: don't inline dispatcher::get_default (tokio-rs#994) core: make `Level` and `LevelFilter` `Copy` (tokio-rs#992)
…spatch-as-ref-tokio-rs#455 * upstream/master: (28 commits) opentelemetry: prepare for 0.8.0 release (tokio-rs#1036) docs: add favicon for extra pretty docs (tokio-rs#1033) subscriber: fix `reload` ergonomics (tokio-rs#1035) chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031) opentelemetry: Assign default ids if missing (tokio-rs#1027) chore: remove deprecated add-path from CI (tokio-rs#1026) attributes: fix `#[instrument(err)]` in case of early returns (tokio-rs#1006) core: remove mandatory liballoc dependency with no-std (tokio-rs#1017) chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023) subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) docs: switch to intra-doc links in tracing-core (tokio-rs#1010) tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007) core: add intrusive linked list for callsite registry (tokio-rs#988) serde: allow tracing-serde to work on no_std. (tokio-rs#960) tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003) tracing: make `Entered` `!Send` (tokio-rs#1001) ...
Motivation
The
Entered
guard returned bySpan::enter
represents entering andexiting a span on the current thread. Calling
Span::enter
enters thespan, returning an
Entered
, and dropping theEntered
ensures thatthe span is exited. This ensures that all spans, once entered, are
eventually exited.
However,
Entered
has an auto-impl ofSend
, because it doesn'tcontain any
!Send
types. This means that theEntered
guard couldbe sent to another thread and dropped. This would cause the original
thread to never exit the span,. and the thread to which the
Entered
guard was sent would exit a span that it never observed an enter for.
This is incorrect.
Solution
This PR adds a
*mut ()
field toEntered
so that it no longerimplements
Send
. There's now a manualSync
impl so structs holdingan
Entered
can still beSync
.Fixes #698
Signed-off-by: Eliza Weisman eliza@buoyant.io