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

backport #2071 and #2073 #2074

Merged
merged 2 commits into from
Apr 14, 2022
Merged

backport #2071 and #2073 #2074

merged 2 commits into from
Apr 14, 2022

Commits on Apr 14, 2022

  1. tracing: fix the "log" feature making async block futures !Send (#2073

    )
    
    ## 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 committed Apr 14, 2022
    Configuration menu
    Copy the full SHA
    f333048 View commit details
    Browse the repository at this point in the history
  2. core: add Value impl for Box<T> where T: Value (#2071)

    This commit adds a `Value` implementation for `Box<T> where T: Value`.
    This is *primarily* intended to make `Box<dyn Error + ...>` implement
    `Value`, building on the `Value` impls for `dyn Error + ...` added in
    #2066, but it may be useful for other boxed values as well.
    
    Refs: #1308
    hawkw committed Apr 14, 2022
    Configuration menu
    Copy the full SHA
    6d68aab View commit details
    Browse the repository at this point in the history