-
Notifications
You must be signed in to change notification settings - Fork 701
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
davidbarsky
approved these changes
Oct 19, 2021
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This backports PR #781 from
master
.Motivation
Currently, the
tracing-subscriber
crate has theMakeWriter
trait forcustomizing the io writer used by
fmt
. This trait is necessary (ratherthan simply using a
Write
instance) because the default implementationperforms 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 sharedWrite
instance,etc).
Right now there is a blanket impl for
Fn() -> T where T: Write
. Thisworks fine with functions like
io::stdout
. However, the other commoncase for this trait is locking a shared writer.
Therefore, it makes sense to see an implementation like this:
Unfortunately, it's impossible to write this. Since
MakeWriter
alwaystakes an
&self
parameter and returnsSelf::Writer
, the genericparameter is unbounded:
This essentially precludes any
MakeWriter
impl where the writer isborrowed from the type implementing
MakeWriter
. This is a significantblow to the usefulness of the trait. For example, it prevented the use
of
MakeWriter
intracing-flame
as suggested in#631 (comment).
Proposal
This PR changes
MakeWriter
to be generic over a lifetime'a
:The
self
parameter is now borrowed for the&'a
lifetime, so it isokay to return a writer borrowed from
self
, such as in theMutex
case.
I've also added an impl of
MakeWriter
forMutex<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