tracing-core, tracing-subscriber: Add {Collect,Subscriber}::on_register_dispatch

[jswrenn]

The on_register_dispatch method is invoked when a Collect is registered as a Dispatch. This method can be overridden to perform actions upon the installation of a collector/subscriber; for instance, to send a copy of the collector's Dispatch to a worker thread.

Unresolved questions:

• Is the signature of on_register_dispatch correct?
  • Should it take &self, or &mut self?
  • Should it take &self, or only the &Dispatch parameter (which can then, if desired, be downcasted to &self)?

[bryangarza]

* [ ]  Is the signature of `on_register_dispatch` correct?
  
  * Should it take `&Dispatch`, or `&mut Dispatch`?
  * Should it take `&self`, or _only_ the `&Dispatch` parameter (which can then, if desired, be downcasted to `&self`)?

I think it can't be &mut Dispatch if multiple Layers are going to receive it.

For the &self question, for now it seems fine to use it. We can remove it later if we see it was unnecessary?

[bryangarza]

@jswrenn I think it's failing CI cause of clippy

[davidbarsky]

I'm not opposed to this change (it is rather small...), but I'd like a little more detail about the motivations for this use-case, as I'm somewhat hesitant to make changes to tracing-core. For instance, I'm curious as to why the use-case of sending a copy of the Collector's Dispatch to a worker thread can't be achieved through, e.g., an Instrument-style extension trait or setting a global collect/dispatch or if there are other motivations at play.

[hawkw]

• Should it take &Dispatch, or &mut Dispatch?

it should not take &mut Dispatch --- there are no methods on Dispatch which require mutable access, so the only thing an &mut Dispatch would allow is for the layer/subscriber to reassign the Dispatch with a different value...which, as far as I can imagine, could only cause problems, and has no practical use case.

[jswrenn]

@hawkw, whoops, I definitely meant to write:

Should it take &self, or &mut self?

An argument in favor of the method consuming &mut self is that it'd mirror Layer::on_layer, which also consumes &mut self. However, the PR currently has the method consume &self, because one way a Dispatch can be created is from Dispatch::from_static, which consumes an &'static (dyn Collect + Send + Sync).

[hawkw]

@hawkw, whoops, I definitely meant to write:

Should it take &self, or &mut self?

An argument in favor of the method consuming &mut self is that it'd mirror Layer::on_layer, which also consumes &mut self. However, the PR currently has the method consume &self, because one way a Dispatch can be created is from Dispatch::from_static, which consumes an &'static (dyn Collect + Send + Sync).

I think the receiver should probably be &self --- there's been some discussion about changing the on_layer API to take &self in 0.4 anyway: #2101.

It's a bummer that we can't just use an &mut self receiver, and would have to introduce locking to store the Dispatch, but I think we need to be able to support Dispatch::from_static. Alternatively, we could consider changing where register_dispatch is actually called: rather than calling it in Dispatch::new, we could call it the first time the Dispatch is set as the default (using Arc::get_mut to access it mutably), but that would fail if other clones exist, which would also be weird and confusing. I think it has to be &self for that reason.

[hawkw]

It occurs to me that adding an API like this has the unfortunate consequence of making it quite easy to accidentally write memory leaks. In your specific use-case in tokio-console, you're sending a clone of the Dispatch to a worker thread, which is fine; however, perhaps the most obvious use of this API would be to create a self-referential Layer that stores a Dispatch referencing the whole stack, so that it can call into the subscriber stack containing it. If this is done, an Arc cycle would be created, and the subscriber would never be dropped.

Currently, there's no public API for turning a Dispatch into a weak reference that could break these cycles, so any use of this API to create a self-referential Layer or Collect implementation is inherently a memory leak...

[jswrenn]

My feeling is that it's probably uncommon to have many complex subscribers initialized and installed over a program's lifetime, so the risk is low. In the near-term, it's sufficient to document this risk. In the long-term, we could add Dispatch::downgrade() to support that use-case.

[davidbarsky]

stamped; needed for console, but this might be removed in 0.2 because of other collector changes.

[davidbarsky]

@hawkw mentioned in discord that she'd like a way to get a weak ref to a dispatcher, but noted that can be done in followup PR.

[hawkw]

@hawkw mentioned in discord that she'd like a way to get a weak ref to a dispatcher, but noted that can be done in followup PR.

Yeah. I would prefer to not release this feature without some way of breaking Dispatch cycles, but I'm happy to merge the PR as-is and add that separately.