Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

subscriber: add lifetime parameter to MakeWriter (#781) #1654

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Oct 19, 2021

This backports PR #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:

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
#631 (comment).

Proposal

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

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 #675.

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

Motivation

Solution

This backports PR #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
#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 #675.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested a review from a team as a code owner October 19, 2021 23:30
@hawkw hawkw merged commit 6cc6c47 into v0.1.x Oct 19, 2021
@hawkw hawkw deleted the eliza/backport-781 branch October 19, 2021 23:45
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
…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 <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants