Skip to content
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

Merged
merged 5 commits into from
Sep 30, 2020
Merged

tracing: make Entered !Send #1001

merged 5 commits into from
Sep 30, 2020

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Sep 29, 2020

Motivation

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.

Solution

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

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>
@hawkw hawkw requested a review from a team as a code owner September 29, 2020 19:35
examples/examples/hyper-echo.rs Outdated Show resolved Hide resolved
@@ -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 ()>,
Copy link

@MikailBag MikailBag Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_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

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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>
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`.
@hawkw hawkw merged commit 515255f into master Sep 30, 2020
@hawkw hawkw added this to the tracing 0.2 milestone Sep 30, 2020
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 7, 2020
* 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)
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 16, 2020
…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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tracing: Entered guards should be !Send
4 participants