backport #2071 and #2073 #2074
Merged
backport #2071 and #2073 #2074
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
) ## Motivation Currently, enabling the `log` feature can cause a compilation error when using `tracing` macros in async functions or blocks. This is because, when the `log` feature is enabled, the `tracing` macros will construct a `log::Record`, which is not `Send`, causing the async fn/block future to become `!Send`. This can break compilation when the future is `Send` without the `log` feature enabled. This is really not great, as it makes the feature no longer purely additive. ## Solution This branch fixes the issue by moving the `log::Record` construction behind a function call. Because the `log::Record` is now only constructed inside of a function, the `log::Record` is now never a local variable within the async block, so it no longer affects the future's auto traits. It's kind of a shame that the compiler can't otherwise determine that the `log::Record` can never be held across an await point, but sticking it in a separate function makes this obvious to the compiler. Also, as a side benefit, this may reduce the size of macro-generated code when the `log` feature is enabled a bit, which could improve performance marginally in some cases. I added two tests ensuring that `async fn` and `async` block futures are `Send` when containing `tracing` macro calls. Previously, on `master`, these failed when the `log` feature is enabled. After this change, the tests now pass. Thanks to @Noah-Kennedy for the original MCRE these tests were based on! Finally, while I was here, I took advantage of the opportunity to clean up the log integration code a bit. Now that the `log::Record` construction is behind a function call in `__macro_support`, the `LogValueSet` type, which is used to wrap a `ValueSet` and implement `fmt::Display` to add it to the textual message of `log::Record`, no longer needs to be exposed to the macros, so it can be made properly private, rather than `#[doc(hidden)] pub`. Also, I noticed that the `span` module implemented its own versions of `LogValueSet` (`FmtAttrs` and `FmtValues`), which were essentially duplicating the same behavior for logging span fields. I removed these and changed this code to use the `LogValueSet` type instead (which will format string `message` fields slightly nicer as well). Fixes #1793 Fixes #1487 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added
kind/bug
hawkw
requested review from
a team,
jtescher,
yaahc and
carllerche
as code owners
hawkw
removed
kind/bug
davidbarsky
approved these changes
Apr 14, 2022
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 the following changes from
master:Valueimpl forBox<T> where T: Value(core: addValueimpl forBox<T> where T: Value#2071)!Send(tracing: fix the "log" feature making async block futures!Send#2073)