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-subscriber: impl LookupSpan for Box<LS> and Arc<LS> #2247

Merged
merged 1 commit into from Jul 26, 2022

Conversation

jswrenn
Copy link
Contributor

@jswrenn jswrenn commented Jul 26, 2022

These implementations mirror those provided by tracing-core for Collect on Box<C> and Arc<C>.

These implementations mirror those provided by tracing-core for
`Collect` on `Box<C>` and `Arc<C>`.
@jswrenn jswrenn requested review from hawkw, davidbarsky and a team as code owners July 26, 2022 21:48
@@ -1656,7 +1656,7 @@ feature! {
}


impl<C, S> Subscribe<C> for Vec<S>
impl<C, S> Subscribe<C> for alloc::vec::Vec<S>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is unrelated, but included here because I noticed that

$ cargo build --no-default-features --features alloc

was failing without it.

@jswrenn
Copy link
Contributor Author

jswrenn commented Jul 26, 2022

@hawkw Would it also be possible to backport this and release in the near future? My only real two blockers for tokio-rs/console#363 are this PR and #2229 (which you've already backported, and is just pending release)!

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks good to me, thank you! i'll take care of the backports

@hawkw hawkw merged commit 876fe1f into tokio-rs:master Jul 26, 2022
hawkw pushed a commit that referenced this pull request Jul 29, 2022
These implementations mirror those provided by tracing-core for
`Collect` on `Box<C>` and `Arc<C>`.
@hawkw hawkw mentioned this pull request Jul 29, 2022
hawkw pushed a commit that referenced this pull request Jul 29, 2022
These implementations mirror those provided by tracing-core for
`Collect` on `Box<C>` and `Arc<C>`.
hawkw pushed a commit that referenced this pull request Jul 29, 2022
These implementations mirror those provided by tracing-core for
`Subscriber` on `Box<S>` and `Arc<S>`.
@jswrenn
Copy link
Contributor Author

jswrenn commented Jul 29, 2022

@hawkw Hm, unfortunately, I just discovered a shortcoming of this PR. Since LookupSpan provides a default implementation of LookupSpan::register_filter, I (incorrectly) did not try to implement it myself. So, this PR provides only the stub implementation of LookupSpan::register_filter, which simply panics. Unfortunately, it's impossible to simply 'forward' the method call to the underlying subscriber, as this PR does for LookupSpan::{span,span_data}, because register_filter consumes an &mut self, and there's no (safe) way to go from &mut Arc<T> to &mut T.

I think the impl provided by this PR is probably better-than-nothing for v0.1.x, but for the master branch, it might be more sensible to provide the impl instead for Arc<RwLock<S>> — or to consider having LookupSpan::register_filter consume &self, instead.

@hawkw
Copy link
Member

hawkw commented Jul 29, 2022

Ah, yeah, I had forgotten about that --- I believe there was actually a similar implementation previously that was removed for the exact reason you're referring to. I think we probably don't want to publish an implementation of LookupSpan for Arc<T> that's broken, so we may need to revert that part of the change.

However, I will note that

there's no (safe) way to go from &mut Arc<T> to &mut T.

isn't actually correct: Arc::get_mut exists, and will (safely) mutably borrow the Arc's contents if it is the only clone of that Arc that exists. Sadly, I don't think an implementation that uses this will be particularly useful in this case, since the Arced LookupSpan type will generally be cloned prior to adding additional Layers to it, as it would be much more difficult to extract just the Arced type to clone it after the Layers are added. If we could trust that code using an Arc<T> subscriber would only clone the Arc by downcasting a whole subscriber stack to an Arc<T> and cloning it only after all layers are added, we could potentially release a LookupSpan impl for Arc<T> that uses Arc::get_mut...but I'm not sure if we can rely on most code that would use this having that shape...

Alternatively, your other suggestion about changing register_filter to take an immutably-borrowed receiver is also worth exploring --- there's a similar discussion around the on_layer callback, where register_filter gets called: #2101 so in tracing-subscriber 0.4, we might end up changing those hooks to take immutably-borrowed receivers...

hawkw added a commit that referenced this pull request Oct 6, 2022
)"

This reverts commit a0824d3 (PR #2247).
As discussed in [this comment][1], the implementation for `Arc`s may
cause subtly incorrect behavior if actually used, due to the `&mut self`
receiver of the `LookupSpan::register_filter` method, since the `Arc`
cannot be mutably borrowed if any clones of it exist.

The APIs added in PRs #2269 and #2293 offer an alternative solution to
the same problems this change was intended to solve, and --- since this
change hasn't been published yet --- it can safely be reverted.

[1]:
    https://giethub.com/tokio-rs/tracing/pull/2247#issuecomment-1199924876
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.

None yet

2 participants