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

v0.1.20 causes issues with Tonic #954

Closed
jtescher opened this issue Aug 24, 2020 · 1 comment · Fixed by #987
Closed

v0.1.20 causes issues with Tonic #954

jtescher opened this issue Aug 24, 2020 · 1 comment · Fixed by #987
Assignees
Labels
crate/tracing Related to the `tracing` crate kind/bug Something isn't working

Comments

@jtescher
Copy link
Collaborator

jtescher commented Aug 24, 2020

Bug Report

Version

  • tracing v0.1.20

Crates

  • tracing

Description

Upgrading from tracing 0.1.19 to 0.1.20 causes tonic to no longer build with the following error:

Reproduction steps:

$ cargo new example && cd example
$ cargo add tonic
$ cargo add tracing
$ cargo build

Build output:

~/.cargo/registry/src/github.com-1ecc6299db9ec823/tower-balance-0.3.0/src/pool/mod.rs:407:21
    |
399 |         let discover = self.balance.discover_mut().as_mut().project();
    |                        ------------ mutable borrow occurs here
...
407 |                     tracing::trace!({ ewma = %self.ewma }, "pool is under-provisioned");
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^----^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                     |                         |
    |                     |                         second borrow occurs due to use of `self` in closure
    |                     immutable borrow occurs here
408 |                 }
409 |                 *discover.load = Level::High;
    |                 ---------------------------- mutable borrow later used here
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

@hawkw hawkw added crate/tracing Related to the `tracing` crate kind/bug Something isn't working labels Aug 24, 2020
@hawkw
Copy link
Member

hawkw commented Aug 24, 2020

Thanks. I think this is not specific to tonic but potentially an issue with the way closures are now used to capture values in #943. This might be an interaction with the code generated by pin-project, or just something we missed with the tests for various kinds of macro invocations.

I went ahead and yanked 0.1.20 so we don't break any more builds until this is fixed.

@hawkw hawkw self-assigned this Aug 24, 2020
hawkw added a commit that referenced this issue Sep 25, 2020
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Sep 25, 2020
## Motivation

In PR #943, the construction of `ValueSet`s for events and spans was
moved out of the code expanded at the callsite and into a closure, in
order to reduce the amount of assembly generated in functions containing
tracing macros. However, this introduced an accidental breaking change
for some dependent crates. Borrowing values inside a closure meant that
when a field of a struct, array, or tuple was used as a field value, the
closure must borrow the _entire_ struct, array, or tuple, rather than
the individual field. This broke code where another unrelated field of
that struct, array, or tuple would then be mutably borrowed or moved
elsewhere. 

## Solution

This branch fixes the breaking change by moving `ValueSet` construction
back out of a closure and into the code expanded at the callsite. This
_does_ regress the amount of assembly generated a bit: a function
containing a single `event!` macro generates 32 instructions in release
mode on master, and after this change, it generates 83 instructions.
However, this is better than reverting PR #943 entirely, which generates
103 instructions for the same function. This change allows us to
continue to benefit from *some* of the changes made in #943, although we
no longer can benefit from the most significant one.

Rather than trying to further optimize the macro expanded code now, I
think we should wait for the `ValueSet` rework that will land in
`tracing` 0.2, where we could potentially generate significantly less
code by virtue of constructing a `ValueSet` with a much simpler array
literal (no `FieldSet` iteration required).

Fixes #954 
Closes #986

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
crate/tracing Related to the `tracing` crate kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants