Skip to content

Commit

Permalink
subscriber: add lifetime parameter to MakeWriter (tokio-rs#781) (to…
Browse files Browse the repository at this point in the history 
…kio-rs#1654)

This backports PR tokio-rs#781 from `master`.

## Motivation

Currently, the `tracing-subscriber` crate has the `MakeWriter` trait for
customizing the io writer used by `fmt`. This trait is necessary (rather
than simply using a `Write` instance) because the default implementation
performs the IO on the thread where an event was recorded, meaning that
a separate writer needs to be acquired by each thread (either by calling
a function like `io::stdout`, by locking a shared `Write` instance,
etc).

Right now there is a blanket impl for `Fn() -> T where T: Write`. This
works fine with functions like `io::stdout`. However, the _other_ common
case for this trait is locking a shared writer.

Therefore, it makes sense to see an implementation like this:

``` rust
impl<'a, W: io::Write> MakeWriter for Mutex<W>
where
    W: io::Write,
{
    type Writer = MutexWriter<'a, W>;
    fn make_writer(&self) -> Self::Writer {
        MutexWriter(self.lock().unwrap())
    }
}

pub struct MutexWriter<'a, W>(MutexGuard<'a, W>);

impl<W: io::Write> io::Write for MutexWriter<'_, W> {
    // write to the shared writer in the `MutexGuard`...
}
```

Unfortunately, it's impossible to write this. Since `MakeWriter` always
takes an `&self` parameter and returns `Self::Writer`, the generic
parameter is unbounded:
```
    Checking tracing-subscriber v0.2.4 (/home/eliza/code/tracing/tracing-subscriber)
error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates
  --> tracing-subscriber/src/fmt/writer.rs:61:6
   |
61 | impl<'a, W: io::Write> MakeWriter for Mutex<W>
   |      ^^ unconstrained lifetime parameter

error: aborting due to previous error
```

This essentially precludes any `MakeWriter` impl where the writer is
borrowed from the type implementing `MakeWriter`. This is a significant
blow to the usefulness of the trait. For example, it prevented the use
of `MakeWriter` in `tracing-flame` as suggested in
tokio-rs#631 (comment).

## Proposal

This PR changes `MakeWriter` to be generic over a lifetime `'a`:

```rust
pub trait MakeWriter<'a> {
    type Writer: io::Write;

    fn make_writer(&'a self) -> Self::Writer;
}
```
The `self` parameter is now borrowed for the `&'a` lifetime, so it is
okay to return a writer borrowed from `self`, such as in the `Mutex`
case.

I've also added an impl of `MakeWriter` for `Mutex<T> where T: Writer`.

Unfortunately, this is a breaking change and will need to wait until we
release `tracing-subscriber` 0.3.

Fixes tokio-rs#675.

Signed-off-by: Eliza Weisman <[email protected]>
  • Loading branch information
@hawkw
hawkw authored Oct 19, 2021
1 parent 4e5f0f0 commit 6cc6c47
Show file tree
Hide file tree
Showing 6 changed files with 359 additions and 418 deletions.
4 changes: 2 additions & 2 deletions tracing-appender/src/non_blocking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,10 +240,10 @@ impl std::io::Write for NonBlocking {
}
}

impl MakeWriter for NonBlocking {
impl<'a> MakeWriter<'a> for NonBlocking {
type Writer = NonBlocking;

fn make_writer(&self) -> Self::Writer {
fn make_writer(&'a self) -> Self::Writer {
self.clone()
}
}
Expand Down
84 changes: 29 additions & 55 deletions tracing-subscriber/src/fmt/fmt_layer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ impl<S, N, E, W> Layer<S, N, E, W>
where
S: Subscriber + for<'a> LookupSpan<'a>,
N: for<'writer> FormatFields<'writer> + 'static,
W: MakeWriter + 'static,
W: for<'writer> MakeWriter<'writer> + 'static,
{
/// Sets the [event formatter][`FormatEvent`] that the layer will use to
/// format events.
Expand Down Expand Up @@ -171,7 +171,7 @@ impl<S, N, E, W> Layer<S, N, E, W> {
/// [`Layer`]: ../layer/trait.Layer.html
pub fn with_writer<W2>(self, make_writer: W2) -> Layer<S, N, E, W2>
where
W2: MakeWriter + 'static,
W2: for<'writer> MakeWriter<'writer> + 'static,
{
Layer {
fmt_fields: self.fmt_fields,
Expand Down Expand Up @@ -477,7 +477,7 @@ where
S: Subscriber + for<'a> LookupSpan<'a>,
N: for<'writer> FormatFields<'writer> + 'static,
E: FormatEvent<S, N> + 'static,
W: MakeWriter + 'static,
W: for<'writer> MakeWriter<'writer> + 'static,
{
/// Builds a [`Layer`] with the provided configuration.
///
Expand Down Expand Up @@ -508,7 +508,7 @@ where
S: Subscriber + for<'a> LookupSpan<'a>,
N: for<'writer> FormatFields<'writer> + 'static,
E: FormatEvent<S, N> + 'static,
W: MakeWriter + 'static,
W: for<'writer> MakeWriter<'writer> + 'static,
{
#[inline]
fn make_ctx<'a>(&'a self, ctx: Context<'a, S>) -> FmtContext<'a, S, N> {
Expand Down Expand Up @@ -589,7 +589,7 @@ where
S: Subscriber + for<'a> LookupSpan<'a>,
N: for<'writer> FormatFields<'writer> + 'static,
E: FormatEvent<S, N> + 'static,
W: MakeWriter + 'static,
W: for<'writer> MakeWriter<'writer> + 'static,
{
fn new_span(&self, attrs: &Attributes<'_>, id: &Id, ctx: Context<'_, S>) {
let span = ctx.span(id).expect("Span not found, this is a bug");
Expand Down Expand Up @@ -923,9 +923,7 @@ mod test {
};
use crate::Registry;
use format::FmtSpan;
use lazy_static::lazy_static;
use regex::Regex;
use std::sync::Mutex;
use tracing::subscriber::with_default;
use tracing_core::dispatcher::Dispatch;

Expand Down Expand Up @@ -982,13 +980,9 @@ mod test {

#[test]
fn synthesize_span_none() {
lazy_static! {
static ref BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
}

let make_writer = || MockWriter::new(&BUF);
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Subscriber::builder()
.with_writer(make_writer)
.with_writer(make_writer.clone())
.with_level(false)
.with_ansi(false)
.with_timer(MockTime)
Expand All @@ -999,19 +993,15 @@ mod test {
let span1 = tracing::info_span!("span1", x = 42);
let _e = span1.enter();
});
let actual = sanitize_timings(String::from_utf8(BUF.try_lock().unwrap().to_vec()).unwrap());
let actual = sanitize_timings(make_writer.get_string());
assert_eq!("", actual.as_str());
}

#[test]
fn synthesize_span_active() {
lazy_static! {
static ref BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
}

let make_writer = || MockWriter::new(&BUF);
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Subscriber::builder()
.with_writer(make_writer)
.with_writer(make_writer.clone())
.with_level(false)
.with_ansi(false)
.with_timer(MockTime)
Expand All @@ -1022,7 +1012,7 @@ mod test {
let span1 = tracing::info_span!("span1", x = 42);
let _e = span1.enter();
});
let actual = sanitize_timings(String::from_utf8(BUF.try_lock().unwrap().to_vec()).unwrap());
let actual = sanitize_timings(make_writer.get_string());
assert_eq!(
"fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: enter\n\
fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: exit\n",
Expand All @@ -1032,13 +1022,9 @@ mod test {

#[test]
fn synthesize_span_close() {
lazy_static! {
static ref BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
}

let make_writer = || MockWriter::new(&BUF);
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Subscriber::builder()
.with_writer(make_writer)
.with_writer(make_writer.clone())
.with_level(false)
.with_ansi(false)
.with_timer(MockTime)
Expand All @@ -1049,7 +1035,7 @@ mod test {
let span1 = tracing::info_span!("span1", x = 42);
let _e = span1.enter();
});
let actual = sanitize_timings(String::from_utf8(BUF.try_lock().unwrap().to_vec()).unwrap());
let actual = sanitize_timings(make_writer.get_string());
assert_eq!(
"fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: close timing timing\n",
actual.as_str()
Expand All @@ -1058,13 +1044,9 @@ mod test {

#[test]
fn synthesize_span_close_no_timing() {
lazy_static! {
static ref BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
}

let make_writer = || MockWriter::new(&BUF);
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Subscriber::builder()
.with_writer(make_writer)
.with_writer(make_writer.clone())
.with_level(false)
.with_ansi(false)
.with_timer(MockTime)
Expand All @@ -1076,7 +1058,7 @@ mod test {
let span1 = tracing::info_span!("span1", x = 42);
let _e = span1.enter();
});
let actual = sanitize_timings(String::from_utf8(BUF.try_lock().unwrap().to_vec()).unwrap());
let actual = sanitize_timings(make_writer.get_string());
assert_eq!(
"span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: close\n",
actual.as_str()
Expand All @@ -1085,13 +1067,9 @@ mod test {

#[test]
fn synthesize_span_full() {
lazy_static! {
static ref BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
}

let make_writer = || MockWriter::new(&BUF);
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Subscriber::builder()
.with_writer(make_writer)
.with_writer(make_writer.clone())
.with_level(false)
.with_ansi(false)
.with_timer(MockTime)
Expand All @@ -1102,7 +1080,7 @@ mod test {
let span1 = tracing::info_span!("span1", x = 42);
let _e = span1.enter();
});
let actual = sanitize_timings(String::from_utf8(BUF.try_lock().unwrap().to_vec()).unwrap());
let actual = sanitize_timings(make_writer.get_string());
assert_eq!(
"fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: new\n\
fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: enter\n\
Expand All @@ -1114,32 +1092,28 @@ mod test {

#[test]
fn make_writer_based_on_meta() {
lazy_static! {
static ref BUF1: Mutex<Vec<u8>> = Mutex::new(vec![]);
static ref BUF2: Mutex<Vec<u8>> = Mutex::new(vec![]);
}
struct MakeByTarget<'a> {
make_writer1: MockMakeWriter<'a>,
make_writer2: MockMakeWriter<'a>,
struct MakeByTarget {
make_writer1: MockMakeWriter,
make_writer2: MockMakeWriter,
}

impl<'a> MakeWriter for MakeByTarget<'a> {
type Writer = MockWriter<'a>;
impl<'a> MakeWriter<'a> for MakeByTarget {
type Writer = MockWriter;

fn make_writer(&self) -> Self::Writer {
fn make_writer(&'a self) -> Self::Writer {
self.make_writer1.make_writer()
}

fn make_writer_for(&self, meta: &Metadata<'_>) -> Self::Writer {
fn make_writer_for(&'a self, meta: &Metadata<'_>) -> Self::Writer {
if meta.target() == "writer2" {
return self.make_writer2.make_writer();
}
self.make_writer()
}
}

let make_writer1 = MockMakeWriter::new(&BUF1);
let make_writer2 = MockMakeWriter::new(&BUF2);
let make_writer1 = MockMakeWriter::default();
let make_writer2 = MockMakeWriter::default();

let make_writer = MakeByTarget {
make_writer1: make_writer1.clone(),
Expand Down
Loading

0 comments on commit 6cc6c47

Please sign in to comment.