From dab43145c4b1272a96efb28c9be7c97289df26a6 Mon Sep 17 00:00:00 2001 From: Jack Wrenn Date: Tue, 30 Aug 2022 19:25:43 +0000 Subject: [PATCH 1/8] tracing-core: Add `Dispatch::downgrade()` and `WeakDispatch` Allows collectors and subscribers to stash their own `Dispatch` without causing a memory leak. --- tracing-core/src/collect.rs | 10 ++++ tracing-core/src/dispatch.rs | 65 +++++++++++++++++++++++++ tracing-subscriber/src/subscribe/mod.rs | 10 ++++ tracing/src/dispatch.rs | 2 +- 4 files changed, 86 insertions(+), 1 deletion(-) diff --git a/tracing-core/src/collect.rs b/tracing-core/src/collect.rs index 2bb4e519d9..17aac11940 100644 --- a/tracing-core/src/collect.rs +++ b/tracing-core/src/collect.rs @@ -79,6 +79,16 @@ use core::ptr::NonNull; /// [`event_enabled`]: Collect::event_enabled pub trait Collect: 'static { /// Invoked when this collector becomes a [`Dispatch`]. + /// + /// ## Avoiding Memory Leaks + /// + /// Collectors should not store their own [`Dispatch`]. Doing so will prevent + /// them from being dropped. Instead, you may call [`Dispatch::downgrade`] + /// to construct a [`WeakDispatch`] (analogous to [`std::sync::Weak`]) that is + /// safe to stash, and can be [upgraded][`WeakDispatch::upgrade`] into a `Dispatch`. + /// + /// [`WeakDispatch`]: crate::dispatch::WeakDispatch + /// [`WeakDispatch::upgrade`]: crate::dispatch::WeakDispatch::upgrade fn on_register_dispatch(&self, collector: &Dispatch) { let _ = collector; } diff --git a/tracing-core/src/dispatch.rs b/tracing-core/src/dispatch.rs index 6e36666e43..cd3bfc6b10 100644 --- a/tracing-core/src/dispatch.rs +++ b/tracing-core/src/dispatch.rs @@ -169,6 +169,18 @@ pub struct Dispatch { collector: &'static (dyn Collect + Send + Sync), } +/// `WeakDispatch` is a version of [`Dispatch`] that holds a non-owning reference +/// to a collector. This collector is accessed calling [`WeakDispatch::upgrade`], +/// which returns an `Option`. +#[derive(Clone)] +pub struct WeakDispatch { + #[cfg(feature = "alloc")] + collector: Kind>, + + #[cfg(not(feature = "alloc"))] + collector: &'static (dyn Collect + Send + Sync), +} + #[cfg(feature = "alloc")] #[derive(Clone)] enum Kind { @@ -577,6 +589,20 @@ impl Dispatch { me } + /// Creates a [`WeakDispatch`] from this `Dispatch`. + #[cfg(feature = "alloc")] + pub fn downgrade(&self) -> WeakDispatch { + #[cfg(feature = "alloc")] + let collector = match &self.collector { + Kind::Global(dispatch) => Kind::Global(*dispatch), + Kind::Scoped(dispatch) => Kind::Scoped(Arc::downgrade(dispatch)), + }; + #[cfg(not(feature = "alloc"))] + let collector = &self.collector; + + WeakDispatch { collector } + } + #[cfg(feature = "std")] pub(crate) fn registrar(&self) -> Registrar { Registrar(match self.collector { @@ -859,6 +885,45 @@ where } } +impl WeakDispatch { + /// Attempts to upgrade the `WeakDispatch` to a `Dispatch`. + pub fn upgrade(&self) -> Option { + #[cfg(feature = "alloc")] + let collector = match &self.collector { + Kind::Global(dispatch) => Some(Kind::Global(*dispatch)), + Kind::Scoped(dispatch) => dispatch.upgrade().map(Kind::Scoped), + }; + #[cfg(not(feature = "alloc"))] + let collector = Some(self.collector); + + collector.map(|collector| Dispatch { collector }) + } +} + +impl fmt::Debug for WeakDispatch { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match &self.collector { + #[cfg(feature = "alloc")] + Kind::Global(collector) => f + .debug_tuple("Dispatch::Global") + .field(&format_args!("{:p}", collector)) + .finish(), + + #[cfg(feature = "alloc")] + Kind::Scoped(collector) => f + .debug_tuple("Dispatch::Scoped") + .field(&format_args!("{:p}", collector)) + .finish(), + + #[cfg(not(feature = "alloc"))] + collector => f + .debug_tuple("Dispatch::Global") + .field(&format_args!("{:p}", collector)) + .finish(), + } + } +} + #[cfg(feature = "std")] impl Registrar { pub(crate) fn upgrade(&self) -> Option { diff --git a/tracing-subscriber/src/subscribe/mod.rs b/tracing-subscriber/src/subscribe/mod.rs index f2b1ec7cbb..9bf9e19bd9 100644 --- a/tracing-subscriber/src/subscribe/mod.rs +++ b/tracing-subscriber/src/subscribe/mod.rs @@ -719,6 +719,16 @@ where /// Performs late initialization when installing this subscriber as a /// [collector]. /// + /// ## Avoiding Memory Leaks + /// + /// Subscribers should not store their own [`Dispatch`]. Doing so will prevent + /// them from being dropped. Instead, you may call [`Dispatch::downgrade`] + /// to construct a [`WeakDispatch`] (analogous to [`std::sync::Weak`]) that is + /// safe to stash, and can be [upgraded][`WeakDispatch::upgrade`] into a `Dispatch`. + /// + /// [`WeakDispatch`]: tracing::dispatch::WeakDispatch + /// [`WeakDispatch::upgrade`]: tracing::dispatch::WeakDispatch::upgrade + /// /// [collector]: tracing_core::Collect fn on_register_dispatch(&self, collector: &Dispatch) { let _ = collector; diff --git a/tracing/src/dispatch.rs b/tracing/src/dispatch.rs index b7214f828c..34cb69d805 100644 --- a/tracing/src/dispatch.rs +++ b/tracing/src/dispatch.rs @@ -148,7 +148,7 @@ pub use tracing_core::dispatch::with_default; #[cfg_attr(docsrs, doc(cfg(feature = "std")))] pub use tracing_core::dispatch::DefaultGuard; pub use tracing_core::dispatch::{ - get_default, set_global_default, Dispatch, SetGlobalDefaultError, + get_default, set_global_default, Dispatch, SetGlobalDefaultError, WeakDispatch, }; /// Private API for internal use by tracing's macros. From 4f671eb054e4fe6490c3a34692230bb36a861d4f Mon Sep 17 00:00:00 2001 From: Jack Wrenn Date: Tue, 6 Sep 2022 20:12:01 +0000 Subject: [PATCH 2/8] fix broken import under feature combo --- tracing-core/src/dispatch.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tracing-core/src/dispatch.rs b/tracing-core/src/dispatch.rs index cd3bfc6b10..913cef51c7 100644 --- a/tracing-core/src/dispatch.rs +++ b/tracing-core/src/dispatch.rs @@ -150,11 +150,13 @@ use core::{ use std::{ cell::{Cell, RefCell, RefMut}, error, - sync::Weak, }; #[cfg(feature = "alloc")] -use alloc::sync::Arc; +use alloc::sync::{ + Arc, + Weak, +}; #[cfg(feature = "alloc")] use core::ops::Deref; From f00a6eaca14fb830d93a8311535728ee5b13fac5 Mon Sep 17 00:00:00 2001 From: Jack Wrenn Date: Tue, 6 Sep 2022 23:02:22 +0000 Subject: [PATCH 3/8] rustfmt --- tracing-core/src/dispatch.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tracing-core/src/dispatch.rs b/tracing-core/src/dispatch.rs index 913cef51c7..2f45b6aba1 100644 --- a/tracing-core/src/dispatch.rs +++ b/tracing-core/src/dispatch.rs @@ -153,10 +153,7 @@ use std::{ }; #[cfg(feature = "alloc")] -use alloc::sync::{ - Arc, - Weak, -}; +use alloc::sync::{Arc, Weak}; #[cfg(feature = "alloc")] use core::ops::Deref; From 03786caa740a5bf8640140020ceb2f858ae06939 Mon Sep 17 00:00:00 2001 From: Jack Wrenn Date: Tue, 20 Sep 2022 16:21:20 +0000 Subject: [PATCH 4/8] clarify `WeakDispatch::upgrade` docs --- tracing-core/src/dispatch.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tracing-core/src/dispatch.rs b/tracing-core/src/dispatch.rs index 2f45b6aba1..8d74e7a584 100644 --- a/tracing-core/src/dispatch.rs +++ b/tracing-core/src/dispatch.rs @@ -885,7 +885,25 @@ where } impl WeakDispatch { - /// Attempts to upgrade the `WeakDispatch` to a `Dispatch`. + /// Attempts to upgrade the `WeakDispatch` to a [`Dispatch`]. + /// + /// Returns `None` if the inner `Dispatch` has already been dropped. + /// + /// ## Examples + /// + /// ``` + /// # use tracing_core::collect::NoCollector; + /// # use tracing_core::dispatch::Dispatch; + /// static COLLECTOR: NoCollector = NoCollector::new(); + /// let strong = Dispatch::new(COLLECTOR); + /// let weak = strong.downgrade(); + /// + /// // The strong here keeps it alive, so we can still access the object. + /// assert!(weak.upgrade().is_some()); + /// + /// drop(strong); // But not any more. + /// assert!(weak.upgrade().is_none()); + /// ``` pub fn upgrade(&self) -> Option { #[cfg(feature = "alloc")] let collector = match &self.collector { From ae9514a2c07b662ca3a5b4286bace61b5e9efe40 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sat, 24 Sep 2022 12:08:49 -0700 Subject: [PATCH 5/8] Apply suggestions from code review --- tracing-core/src/collect.rs | 18 +++++++++++++----- tracing-core/src/dispatch.rs | 11 +++++------ tracing-subscriber/src/subscribe/mod.rs | 22 +++++++++++++++------- 3 files changed, 33 insertions(+), 18 deletions(-) diff --git a/tracing-core/src/collect.rs b/tracing-core/src/collect.rs index 17aac11940..ced701d21c 100644 --- a/tracing-core/src/collect.rs +++ b/tracing-core/src/collect.rs @@ -82,13 +82,21 @@ pub trait Collect: 'static { /// /// ## Avoiding Memory Leaks /// - /// Collectors should not store their own [`Dispatch`]. Doing so will prevent - /// them from being dropped. Instead, you may call [`Dispatch::downgrade`] - /// to construct a [`WeakDispatch`] (analogous to [`std::sync::Weak`]) that is - /// safe to stash, and can be [upgraded][`WeakDispatch::upgrade`] into a `Dispatch`. + /// Collectors should not store their own [`Dispatch`]. Because the + /// `Dispatch` owns the collector, storing the `Dispatch` within the + /// collector will create a reference count cycle, preventing the `Dispatch` + /// from ever being dropped. + /// + /// Instead, when it is necessary to store a cyclical reference to the + /// `Dispatch` within a collector, use [`Dispatch::downgrade`] to convert a + /// `Dispatch` into a [`WeakDispatch`]. This type is analogous to + /// [`std::sync::Weak`], and does not create a reference count cycle. A + /// [`WeakDispatch`] can be stored within a collector without causing a + /// memory leak, and can be [upgraded] into a `Dispatch` temporarily when + /// the `Dispatch` must be accessed by the collector. /// /// [`WeakDispatch`]: crate::dispatch::WeakDispatch - /// [`WeakDispatch::upgrade`]: crate::dispatch::WeakDispatch::upgrade + /// [upgraded]: crate::dispatch::WeakDispatch::upgrade fn on_register_dispatch(&self, collector: &Dispatch) { let _ = collector; } diff --git a/tracing-core/src/dispatch.rs b/tracing-core/src/dispatch.rs index 8d74e7a584..4ee4807d1f 100644 --- a/tracing-core/src/dispatch.rs +++ b/tracing-core/src/dispatch.rs @@ -589,7 +589,6 @@ impl Dispatch { } /// Creates a [`WeakDispatch`] from this `Dispatch`. - #[cfg(feature = "alloc")] pub fn downgrade(&self) -> WeakDispatch { #[cfg(feature = "alloc")] let collector = match &self.collector { @@ -885,9 +884,9 @@ where } impl WeakDispatch { - /// Attempts to upgrade the `WeakDispatch` to a [`Dispatch`]. + /// Attempts to upgrade this `WeakDispatch` to a [`Dispatch`]. /// - /// Returns `None` if the inner `Dispatch` has already been dropped. + /// Returns `None` if the referenced `Dispatch` has already been dropped. /// /// ## Examples /// @@ -922,19 +921,19 @@ impl fmt::Debug for WeakDispatch { match &self.collector { #[cfg(feature = "alloc")] Kind::Global(collector) => f - .debug_tuple("Dispatch::Global") + .debug_tuple("WeakDispatch::Global") .field(&format_args!("{:p}", collector)) .finish(), #[cfg(feature = "alloc")] Kind::Scoped(collector) => f - .debug_tuple("Dispatch::Scoped") + .debug_tuple("WeakDispatch::Scoped") .field(&format_args!("{:p}", collector)) .finish(), #[cfg(not(feature = "alloc"))] collector => f - .debug_tuple("Dispatch::Global") + .debug_tuple("WeakDispatch::Global") .field(&format_args!("{:p}", collector)) .finish(), } diff --git a/tracing-subscriber/src/subscribe/mod.rs b/tracing-subscriber/src/subscribe/mod.rs index 9bf9e19bd9..45f65d4a6c 100644 --- a/tracing-subscriber/src/subscribe/mod.rs +++ b/tracing-subscriber/src/subscribe/mod.rs @@ -721,13 +721,21 @@ where /// /// ## Avoiding Memory Leaks /// - /// Subscribers should not store their own [`Dispatch`]. Doing so will prevent - /// them from being dropped. Instead, you may call [`Dispatch::downgrade`] - /// to construct a [`WeakDispatch`] (analogous to [`std::sync::Weak`]) that is - /// safe to stash, and can be [upgraded][`WeakDispatch::upgrade`] into a `Dispatch`. - /// - /// [`WeakDispatch`]: tracing::dispatch::WeakDispatch - /// [`WeakDispatch::upgrade`]: tracing::dispatch::WeakDispatch::upgrade + /// Subscribers should not store the [`Dispatch`] pointing to the collector + /// that they are a part of. Because the `Dispatch` owns the collector, + /// storing the `Dispatch` within the collector will create a reference + /// count cycle, preventing the `Dispatch` from ever being dropped. + /// + /// Instead, when it is necessary to store a cyclical reference to the + /// `Dispatch` within a subscriber, use [`Dispatch::downgrade`] to convert a + /// `Dispatch` into a [`WeakDispatch`]. This type is analogous to + /// [`std::sync::Weak`], and does not create a reference count cycle. A + /// [`WeakDispatch`] can be stored within a subscriber without causing a + /// memory leak, and can be [upgraded] into a `Dispatch` temporarily when + /// the `Dispatch` must be accessed by the subscriber. + /// + /// [`WeakDispatch`]: crate::dispatch::WeakDispatch + /// [upgraded]: crate::dispatch::WeakDispatch::upgrade /// /// [collector]: tracing_core::Collect fn on_register_dispatch(&self, collector: &Dispatch) { From 4a2dbdaed5aaa23fb9d05533784595e0e0582593 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sat, 24 Sep 2022 12:26:44 -0700 Subject: [PATCH 6/8] more docs --- tracing-core/src/dispatch.rs | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/tracing-core/src/dispatch.rs b/tracing-core/src/dispatch.rs index 4ee4807d1f..49e0dc861c 100644 --- a/tracing-core/src/dispatch.rs +++ b/tracing-core/src/dispatch.rs @@ -169,8 +169,24 @@ pub struct Dispatch { } /// `WeakDispatch` is a version of [`Dispatch`] that holds a non-owning reference -/// to a collector. This collector is accessed calling [`WeakDispatch::upgrade`], -/// which returns an `Option`. +/// to a [collector]. +/// +/// The collector may be accessed by calling [`WeakDispatch::upgrade`], +/// which returns an `Option`. If all [`Dispatch`] clones that point +/// at the collector have been dropped, [`WeakDispatch::upgrade`] will return +/// `None`. Otherwise, it will return `Some(Dispatch)`. +/// +/// A `WeakDispatch` may be created from a [`Dispatch`] by calling the +/// [`Dispatch::downgrade`] method. The primary use for creating a +/// [`WeakDispatch`] is to allow a collector to hold a cyclical reference to +/// itself without creating a memory leak. See [here] for details. +/// +/// This type is analogous to the [`std::sync::Weak`] type, but for a +/// [`Dispatch`] rather than an [`Arc`]. +/// +/// [collector]: Collect +/// [`Arc`]: std::sync::Arc +/// [here]: Collect#avoiding-memory-leaks #[derive(Clone)] pub struct WeakDispatch { #[cfg(feature = "alloc")] @@ -589,6 +605,20 @@ impl Dispatch { } /// Creates a [`WeakDispatch`] from this `Dispatch`. + /// + /// A [`WeakDispatch`] is similar to a [`Dispatch`], but it does not prevent + /// the underlying [collector] from being dropped. Instead, it only permits + /// access while other references to the collector exist. This is equivalent + /// to the standard library's [`Arc::downgrade`] method, but for `Dispatch` + /// rather than `Arc`. + /// + /// The primary use for creating a [`WeakDispatch`] is to allow a collector + /// to hold a cyclical reference to itself without creating a memory leak. + /// See [here] for details. + /// + /// [collector]: Collect + /// [`Arc::downgrade`]: std::sync::Arc::downgrade + /// [here]: Collect#avoiding-memory-leaks pub fn downgrade(&self) -> WeakDispatch { #[cfg(feature = "alloc")] let collector = match &self.collector { From 8cba18662b4941cde78624fe0df6eab882759389 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sat, 24 Sep 2022 12:34:12 -0700 Subject: [PATCH 7/8] fix compilation failure without `alloc` This was creating an `&&'static (dyn Collect + ...)` rather than copying the `&'static (dyn Collect + ...)` into the `WeakDispatch` struct, and the reference to a static reference to a `dyn Collect + ...` doesn't forward trait impls. Removing the `&` un-breaks this. --- tracing-core/src/dispatch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tracing-core/src/dispatch.rs b/tracing-core/src/dispatch.rs index 49e0dc861c..9c696ed58c 100644 --- a/tracing-core/src/dispatch.rs +++ b/tracing-core/src/dispatch.rs @@ -626,7 +626,7 @@ impl Dispatch { Kind::Scoped(dispatch) => Kind::Scoped(Arc::downgrade(dispatch)), }; #[cfg(not(feature = "alloc"))] - let collector = &self.collector; + let collector = self.collector; WeakDispatch { collector } } From d050ae859a87b2aa276588e7bf60dfe77cb76104 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sat, 24 Sep 2022 13:03:18 -0700 Subject: [PATCH 8/8] fix tracing-subscriber docs using crate:: for stuff from core --- tracing-subscriber/src/subscribe/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tracing-subscriber/src/subscribe/mod.rs b/tracing-subscriber/src/subscribe/mod.rs index 45f65d4a6c..6d77eb0579 100644 --- a/tracing-subscriber/src/subscribe/mod.rs +++ b/tracing-subscriber/src/subscribe/mod.rs @@ -734,9 +734,8 @@ where /// memory leak, and can be [upgraded] into a `Dispatch` temporarily when /// the `Dispatch` must be accessed by the subscriber. /// - /// [`WeakDispatch`]: crate::dispatch::WeakDispatch - /// [upgraded]: crate::dispatch::WeakDispatch::upgrade - /// + /// [`WeakDispatch`]: tracing_core::dispatch::WeakDispatch + /// [upgraded]: tracing_core::dispatch::WeakDispatch::upgrade /// [collector]: tracing_core::Collect fn on_register_dispatch(&self, collector: &Dispatch) { let _ = collector;