tracing: tracing 0.1.38 included accidentally-breaking change from added Drop impl

Bug Report

Version

Builds fine:

tracing-regression v0.1.0 (/local/home/jongje/dev/tmp/tracing-regression)
└── tracing v0.1.37
    ├── tracing-attributes v0.1.24 (proc-macro)
    └── tracing-core v0.1.30

Is broken:

tracing-regression v0.1.0 (/local/home/jongje/dev/tmp/tracing-regression)
└── tracing v0.1.38
    ├── tracing-attributes v0.1.24 (proc-macro)
    └── tracing-core v0.1.30

Platform

Linux host 5.4.235-151.344.amzn2int.aarch64 #1 SMP Sat Mar 11 23:52:03 UTC 2023 aarch64 aarch64 aarch64 GNU/Linux

Crates

tracing and tracing-attributes (I believe a Drop impl was added in the latter as well)

Description

The change in https://github.com/tokio-rs/tracing/pull/2562 that was intended to fix https://github.com/tokio-rs/tracing/issues/2541 is a breaking change (and indeed broke my code). In particular, we now have https://github.com/ilslv/tracing/blob/73e73a9d7677a848c614c976ac176d56c889bb9b/tracing/src/instrument.rs#L307

whereas we didn’t before. If you have impl Drop for type with a generic type parameter T, Rust assumes that the type T may be used in the Drop implementation, and therefore requires that T outlives the wrapper type. This wasn’t previously the case — without the Drop, the T only had to live as long as the last use of the wrapper, but crucially not all the way until it is dropped. You can see this with code like:

use std::task::{Context, Poll};
use tower_service::Service;
use tracing::instrument::Instrument;

pub struct Svc;

impl tower_service::Service<()> for Svc {
    type Response = ();
    type Error = ();
    type Future = std::future::Ready<Result<Self::Response, Self::Error>>;

    fn poll_ready(&mut self, _: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        Poll::Ready(Ok(()))
    }

    fn call(&mut self, _: ()) -> Self::Future {
        std::future::ready(Ok(()))
    }
}

pub async fn foo(s: tracing::Span, f: &mut Svc) {
    let mut _ready = f.ready().instrument(s.clone());
    // _ready is no longer used from this point on
    //
    // The type of _ready is Instrumented<Ready<&mut Svc>>
    // Rust only permits the &mut Svc to be used if
    // neither Ready nor Instrumented have a Drop impl.
    // This is because otherwise the implicit drop(_ready)
    // below might access &mut Svc, in which case any use
    // in the interrim would be unsound.
    //
    // (
    //   Alternatively, `impl Drop` is permitted if
    //   each <T> is marked #[may_dangle] as per
    //   https://github.com/rust-lang/rust/issues/34761
    // )
    let _ = s.in_scope(|| f.call(()));
    // implicit drop(_ready)
}

trait ServiceExt {
    fn ready(&mut self) -> std::future::Ready<&'_ mut Self>
    where
        Self: Sized,
    {
        std::future::ready(self)
    }
}

impl<T> ServiceExt for T where T: Service<()> {}

which will build fine with tracing 0.1.37, but no longer builds with tracing 0.1.38 (which has this change):

error[E0500]: closure requires unique access to `*f` but it is already borrowed
  --> src/lib.rs:37:24
   |
22 |     let mut _ready = f.ready().instrument(s.clone());
   |                      --------- borrow occurs here
...
37 |     let _ = s.in_scope(|| f.call(()));
   |                        ^^ - second borrow occurs due to use of `*f` in closure
   |                        |
   |                        closure construction occurs here
38 |     // implicit drop(_ready)
39 | }
   | - first borrow might be used here, when `_ready` is dropped and runs the `Drop` code for type `Instrumented`

This likely warrants a yank if enough people run into it. It could just be me, though this was in fairly innocuous code. Tracing it back to this change also wasn’t trivial, so some folks may run into this but not realize how the issue arose for them in the first place.

The change will either need to be rolled back, or we’ll need to include a #[may_dangle] annotation on the added impl Drop.

About this issue

  • Original URL
  • State: open
  • Created a year ago
  • Reactions: 2
  • Comments: 19 (13 by maintainers)

Commits related to this issue

Most upvoted comments

Yeah, I think it’s just tracing-futures (because of this impl) and tracing (because of this impl) that need to be yanked. I think I just saw tracing-attributes in the file diff list and assumed it was affected too.

As for #[may_dangle], the big problem is that it’s still unstable (so nightly only), which means you’d still be breaking anyone not on nightly. The feature and its use is discussed in some detail in the Nomicon (and in one of my videos :p), but it’s still in flux (the new RFC text is actually pretty helpful), so relying on it even on nightly is probably not a great call right now. Ultimately though, what you’d want is something like:

unsafe impl<#[may_dangle(droppable)] T> PinnedDrop for Instrumented<T> {

This may also end up requiring support from pin_project when that day comes.

The unsafe here is needed to assert that the drop impl indeed does not access T in any way except to drop it.

Which is all to say, I think our options are:

  1. Land this as-is and hope that the breakage is minimal.
  2. Cut a breaking release of tracing and tracing-futures.
  3. Revert the change and re-introduce it when #[may_dangle] is stabilized.

What’s kind of annoying about option 2 is that you really do want the #[may_dangle] annotation here — without it, users of the library will find that entirely reasonable code (like what I gave above) does not compile. So if we cut a new major version of these crates, users would end up with worse ergonomics (at least in this one way) until such time as #[may_dangle] stabilizes, at which point we could start using it, and at which point the breaking change would no longer be necessary in the first place 😅

You have a better idea than I do about how valuable the change in #2562 is, and whether it’s okay to delay or not.

+6
jonhoo on Apr 29, 2023

I went ahead and yanked v0.1.38 until we figure out a long-term fix.

+6
hawkw on Apr 29, 2023

Sorry, I don’t have the time to dig into this currently, but I hope the documentation added by https://github.com/rust-lang/rust/pull/103413 helps. AFAIK currently making a type Drop is technically a breaking change even if the type is 'static, i.e. there are examples where adding a String to a tuple introduced dropck failures. There are also people who are unhappy about this and want to fix it, but I don’t know if that fix is sufficient to make this a non-breaking change. Some links for further reading:

There is no point in adding a bound to the Drop impl. Such bounds are generally rejected, and if they are accepted here then that can only be because the bound is a NOP.

Of course the other question is whether adding a Drop here is likely to break anything in practice. But I guess this issue shows that the answer is “yes”. But then why was there a question above whether there is a breaking change if we have a real-world example that got broken? I am missing some context here from not having the time to read the entire thread, it seems… sorry if what I said makes little sense.^^

+5
RalfJung on May 17, 2023

It’s a good question. I think T: 'static already existing as a bound would be enough to make this not breaking. The added Drop impl only affects the drop check, and the drop check only shows up in the form of lifetime errors, so if all captured lifetimes are already static, I think no new errors could arise. @RalfJung would probably be a better person to ask.

+3
jonhoo on May 14, 2023

I think the way to detect it is if a Drop impl is ever added to a generic type. That’s all it takes if I’m not mistaken (which it could totally be that I am). In practice it often won’t break people because non static things don’t end up in the generics, but if they ever do then you hit this. Unless the unstable #[may_dangle] is also added that is.

+1
jonhoo on May 14, 2023

One potentially useful data point: a (small) test internally at $work only revealed a single code instance (my code) that got bitten by this backwards-incompatibility. I could run a broader sweep internally to see if I find anything else that breaks if that’d be useful.

+1
jonhoo on Apr 30, 2023

Sorry I mistook some other linked PRs as fix.

Really sorry about that, I was probably too sleepy.

+1
NobodyXu on Apr 29, 2023
Read more comments on GitHub
← tracing: Instrument attribute with async-trait breaks include_str
tracing: Unable to dynamically configure with builder →