Skip to content

Commit

Permalink
fix off hints still overriding when None is inside
Browse files Browse the repository at this point in the history
  • Loading branch information
hawkw committed Sep 26, 2022
1 parent c692670 commit 870ba11
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 5 deletions.
20 changes: 15 additions & 5 deletions tracing-subscriber/src/subscribe/layered.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ use core::{
ptr::NonNull,
};

use super::subscriber_is_none;

/// A [collector] composed of a [collector] wrapped by one or more
/// [subscriber]s.
///
Expand Down Expand Up @@ -119,6 +121,7 @@ where
self.pick_level_hint(
self.subscriber.max_level_hint(),
self.inner.max_level_hint(),
super::collector_is_none(&self.inner),
)
}

Expand Down Expand Up @@ -274,6 +277,7 @@ where
self.pick_level_hint(
self.subscriber.max_level_hint(),
self.inner.max_level_hint(),
super::subscriber_is_none(&self.inner),
)
}

Expand Down Expand Up @@ -374,10 +378,9 @@ where
.and(self.inner.downcast_raw(id)),

// Otherwise, try to downcast both branches normally...
_ => self
.subscriber
.downcast_raw(id)
.or_else(|| self.inner.downcast_raw(id)),
_ => {
dbg!(self.subscriber.downcast_raw(id)).or_else(|| dbg!(self.inner.downcast_raw(id)))
}
}
}
}
Expand Down Expand Up @@ -477,6 +480,7 @@ where
&self,
outer_hint: Option<LevelFilter>,
inner_hint: Option<LevelFilter>,
inner_is_none: bool,
) -> Option<LevelFilter> {
if self.inner_is_registry {
return outer_hint;
Expand Down Expand Up @@ -509,10 +513,16 @@ where
// Also note that this does come at some perf cost, but
// this function is only called on initialization and
// subscriber reloading.
if super::subscriber_is_none(&self.subscriber) {
if dbg!(super::subscriber_is_none(&self.subscriber)) {
return cmp::max(outer_hint, Some(inner_hint?));
}

// Similarly, if the layer on the inside is `None` and it returned an
// `Off` hint, we want to override that with the outer hint.
if inner_is_none && inner_hint == Some(LevelFilter::OFF) {

This comment has been minimized.

Copy link
@guswynn

guswynn Sep 26, 2022

Contributor

Oh this also fixes layer that aren't None right, but happen to return OFF?

return outer_hint;
}

cmp::max(outer_hint, inner_hint)
}
}
Expand Down
18 changes: 18 additions & 0 deletions tracing-subscriber/src/subscribe/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1515,6 +1515,24 @@ where
.is_some()
}

/// Is a type implementing `Collect` `Option::<_>::None`?
pub(crate) fn collector_is_none<C>(collector: &C) -> bool
where
C: Collect,
{
unsafe {
// Safety: we're not actually *doing* anything with this pointer ---
// this only care about the `Option`, which is essentially being used
// as a bool. We can rely on the pointer being valid, because it is
// a crate-private type, and is only returned by the `Subscribe` impl
// for `Option`s. However, even if the subscriber *does* decide to be
// evil and give us an invalid pointer here, that's fine, because we'll
// never actually dereference it.
collector.downcast_raw(TypeId::of::<NoneLayerMarker>())
}
.is_some()
}

impl<S, C> Subscribe<C> for Option<S>
where
S: Subscribe<C>,
Expand Down

0 comments on commit 870ba11

Please sign in to comment.