subscriber: fix missing `on_layer` for `Box`/`Arc` layer impls #1576

hawkw · 2021-09-17T20:19:29Z

The Layer::on_layer method on Layer was added in PR #1523. PR #1536,
which added Layer implementations to Box<dyn Layer<...> + ...> and
Arc<dyn Layer<...> + ...>, merged prior to #1523. However, when
merging #1523, I didn't think to update the Layer impl for Boxed and
Arced layers to forward on_layer to the inner Layer. This means
that when a Layer is wrapped in an Arc or a Box, the on_layer
method never gets called.

When per-layer filters are in use, the on_layer method is necessary to
ensure the filter is registered with the inner subscriber and has a
valid ID. This bug means that when per-layer filters are wrapped in a
Box or Arc, they won't be registered, and per-layer filtering
breaks.

This PR fixes the bug by adding on_layer implementations to the
Layer impls for Arced and Boxed layers. I also added some tests
--- thanks to @Noah-Kennedy for the original repro that these were based
on (#1563 (comment)).

I also added a nicer debug assertion to Filtered for cases where
Layer impls don't call on_layer, so that this fails less
confusingly in the future.

this is a minimization of #1563 (comment) Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

@Noah-Kennedy

The `Layer::on_layer` method on `Layer` was added in PR #1523. PR #1536, which added `Layer` implementations to `Box<dyn Layer<...> + ...>` and `Arc<dyn Layer<...> + ...>`, merged prior to #1523. However, when merging #1523, I didn't think to update the `Layer` impl for `Box`ed and `Arc`ed layers to forward `on_layer` to the inner `Layer`. This means that when a `Layer` is wrapped in an `Arc` or a `Box`, the `on_layer` method never gets called. When per-layer filters are in use, the `on_layer` method is necessary to ensure the filter is registered with the inner subscriber and has a valid ID. This bug means that when per-layer filters are wrapped in a `Box` or `Arc`, they won't be registered, and per-layer filtering breaks. This PR fixes the bug by adding `on_layer` implementations to the `Layer` impls for `Arc`ed and `Box`ed layers. I also added some tests --- thanks to @Noah-Kennedy for the original repro that these were based on (#1563 (comment)). Signed-off-by: Eliza Weisman <eliza@buoyant.io>

this would help if this ever happened again Signed-off-by: Eliza Weisman <eliza@buoyant.io>

jsgf · 2021-09-18T05:45:10Z

tracing-subscriber/src/layer/mod.rs

+            // XXX(eliza): this may behave weird if another `Arc` clone of this
+            // layer is layered onto a _different_ subscriber...but there's no
+            // good solution for that...


I don't understand this comment. Arc::get_mut will only return a value if there are no other aliases so if it gets here there can't be a different subscriber. So the weird behaviour will be if there are multiple references to the Layer then this won't get called.

It seems like Arc<Layer> and on_layer are intrinsically incompatible. If having on_layer(&mut self, ... is useful, then maybe there shouldn't be Arc after all?

Arc::get_mut will only return a value if there are no other aliases so if it gets here there can't be a different subscriber.

was thinking about a situation where the Arc is cloned after on_layer is called. But, in practice, that will never happen, because on_layer is only called when layering, and layering moves the layer by value. However, there's nothing stopping user code from just arbitrarily calling on_layer.

It seems like Arc<Layer> and on_layer are intrinsically incompatible. If having on_layer(&mut self, ... is useful, then maybe there shouldn't be Arc after all?

Yes, I'm thinking that as well --- I regret not thinking through the potential implications of the Arc impl w.r.t having an on_layer callback. We may want to deprecate the Arc impls...which doesn't feel great, since we just added them...

...ugh, i've been once again reminded that you can't deprecate a trait impl for a type:

so, I guess the only real solution for this is a big warning in the docs and removing it in the next breaking change. Gah!

Better to deprecate a bad idea earlier than later. And if Arc isn't actually usable, then presumably that will dissuade people from using it.

@connec

# 0.2.24 (September 19, 2021) This release contains a number of bug fixes, including a fix for `tracing-subscriber` failing to compile on the minimum supported Rust version of 1.42.0. It also adds `IntoIterator` implementations for the `Targets` type. ### Fixed - Fixed compilation on Rust 1.42.0 ([#1580], [#1581]) - **registry**: Ensure per-layer filter `enabled` state is cleared when a global filter short-circuits filter evaluation ([#1575]) - **layer**: Fixed `Layer::on_layer` not being called for `Box`ed `Layer`s, which broke per-layer filtering ([#1576]) ### Added - **filter**: Added `Targets::iter`, returning an iterator over the set of target-level pairs enabled by a `Targets` filter ([#1574]) - **filter**: Added `IntoIterator` implementations for `Targets` and `&Targets` ([#1574]) Thanks to new contributor @connec for contributing to this release! [#1580]: #1580 [#1581]: #1581 [#1575]: #1575 [#1576]: #1576 [#1574]: #1574

@connec

# 0.2.24 (September 19, 2021) This release contains a number of bug fixes, including a fix for `tracing-subscriber` failing to compile on the minimum supported Rust version of 1.42.0. It also adds `IntoIterator` implementations for the `Targets` type. ### Fixed - Fixed compilation on Rust 1.42.0 ([#1580], [#1581]) - **registry**: Ensure per-layer filter `enabled` state is cleared when a global filter short-circuits filter evaluation ([#1575]) - **layer**: Fixed `Layer::on_layer` not being called for `Box`ed `Layer`s, which broke per-layer filtering ([#1576]) ### Added - **filter**: Added `Targets::iter`, returning an iterator over the set of target-level pairs enabled by a `Targets` filter ([#1574]) - **filter**: Added `IntoIterator` implementations for `Targets` and `&Targets` ([#1574]) Thanks to new contributor @connec for contributing to this release! [#1580]: #1580 [#1581]: #1581 [#1575]: #1575 [#1576]: #1576 [#1574]: #1574

Implementing `Subscribe` for `Arc`s, which are immutable, breaks the ability to implement `Subscribe::on_layer` with a mutable reference. This is necessary for per-layer filtering. See #1576 (comment) for details. Therefore, the `Layer` impls for `Arc`s should not be used. In 0.3, we have the opportunity to remove these APIs. Therefore, this PR removes them.

Implementing `Layer` for `Arc`s, which are immutable, breaks the ability to implement `Layer::on_layer` with a mutable reference. This is necessary for per-layer filtering. See #1576 (comment) for details. Therefore, the `Layer` impls for `Arc`s should not be used. In 0.3, we have the opportunity to remove these APIs. Therefore, this PR removes them.

@Noah-Kennedy

…-rs#1576) The `Layer::on_layer` method on `Layer` was added in PR tokio-rs#1523. PR tokio-rs#1536, which added `Layer` implementations to `Box<dyn Layer<...> + ...>` and `Arc<dyn Layer<...> + ...>`, merged prior to tokio-rs#1523. However, when merging tokio-rs#1523, I didn't think to update the `Layer` impl for `Box`ed and `Arc`ed layers to forward `on_layer` to the inner `Layer`. This means that when a `Layer` is wrapped in an `Arc` or a `Box`, the `on_layer` method never gets called. When per-layer filters are in use, the `on_layer` method is necessary to ensure the filter is registered with the inner subscriber and has a valid ID. This bug means that when per-layer filters are wrapped in a `Box` or `Arc`, they won't be registered, and per-layer filtering breaks. This PR fixes the bug by adding `on_layer` implementations to the `Layer` impls for `Arc`ed and `Box`ed layers. I also added some tests --- thanks to @Noah-Kennedy for the original repro that these were based on (tokio-rs#1563 (comment)). I also added a nicer debug assertion to `Filtered` for cases where `Layer` impls don't call `on_layer`, so that this fails less confusingly in the future. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

@connec

# 0.2.24 (September 19, 2021) This release contains a number of bug fixes, including a fix for `tracing-subscriber` failing to compile on the minimum supported Rust version of 1.42.0. It also adds `IntoIterator` implementations for the `Targets` type. ### Fixed - Fixed compilation on Rust 1.42.0 ([tokio-rs#1580], [tokio-rs#1581]) - **registry**: Ensure per-layer filter `enabled` state is cleared when a global filter short-circuits filter evaluation ([tokio-rs#1575]) - **layer**: Fixed `Layer::on_layer` not being called for `Box`ed `Layer`s, which broke per-layer filtering ([tokio-rs#1576]) ### Added - **filter**: Added `Targets::iter`, returning an iterator over the set of target-level pairs enabled by a `Targets` filter ([tokio-rs#1574]) - **filter**: Added `IntoIterator` implementations for `Targets` and `&Targets` ([tokio-rs#1574]) Thanks to new contributor @connec for contributing to this release! [tokio-rs#1580]: tokio-rs#1580 [tokio-rs#1581]: tokio-rs#1581 [tokio-rs#1575]: tokio-rs#1575 [tokio-rs#1576]: tokio-rs#1576 [tokio-rs#1574]: tokio-rs#1574

Implementing `Layer` for `Arc`s, which are immutable, breaks the ability to implement `Layer::on_layer` with a mutable reference. This is necessary for per-layer filtering. See tokio-rs#1576 (comment) for details. Therefore, the `Layer` impls for `Arc`s should not be used. In 0.3, we have the opportunity to remove these APIs. Therefore, this PR removes them.

hawkw added 5 commits September 17, 2021 12:06

add repro

d1c0afa

this is a minimization of #1563 (comment) Signed-off-by: Eliza Weisman <eliza@buoyant.io>

nicer repro now that i know what the bug is

253c266

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

fix test crosstalk

251ecfa

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

add assertion for a less confusing panic

72f7642

this would help if this ever happened again Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw requested a review from a team as a code owner September 17, 2021 20:19

hawkw self-assigned this Sep 17, 2021

hawkw requested a review from davidbarsky September 17, 2021 20:19

davidbarsky approved these changes Sep 17, 2021

View reviewed changes

hawkw merged commit e597953 into v0.1.x Sep 17, 2021

hawkw deleted the eliza/more-fun-panics branch September 17, 2021 21:25

jsgf reviewed Sep 18, 2021

View reviewed changes

hawkw mentioned this pull request Sep 19, 2021

subscriber: prepare to release 0.2.24 #1582

Merged

hawkw mentioned this pull request Oct 18, 2021

[Bug] the tracing_subscriber with_filter method causes a panic #1563

Closed

hawkw mentioned this pull request Oct 19, 2021

subscriber: remove Subscribe impls for Arcs #1649

Merged

davidbarsky mentioned this pull request Sep 26, 2023

chore: backport roughly a year's worth of changes #2728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subscriber: fix missing `on_layer` for `Box`/`Arc` layer impls #1576

subscriber: fix missing `on_layer` for `Box`/`Arc` layer impls #1576

hawkw commented Sep 17, 2021

jsgf Sep 18, 2021 •

edited

Loading

hawkw Sep 18, 2021

hawkw Sep 18, 2021

jsgf Sep 25, 2021

subscriber: fix missing on_layer for Box/Arc layer impls #1576

subscriber: fix missing on_layer for Box/Arc layer impls #1576

Conversation

hawkw commented Sep 17, 2021

jsgf Sep 18, 2021 • edited Loading

Choose a reason for hiding this comment

hawkw Sep 18, 2021

Choose a reason for hiding this comment

hawkw Sep 18, 2021

Choose a reason for hiding this comment

jsgf Sep 25, 2021

Choose a reason for hiding this comment

subscriber: fix missing `on_layer` for `Box`/`Arc` layer impls #1576

subscriber: fix missing `on_layer` for `Box`/`Arc` layer impls #1576

jsgf Sep 18, 2021 •

edited

Loading