tokio-rs / tracing

Application level tracing for Rust.
https://tracing.rs
MIT License
5.19k stars 677 forks source link

Incorrect `Interest::Never` is cached and never invalidated on v0.1 #2874

Open plietar opened 5 months ago

plietar commented 5 months ago

Bug Report

Version

tracing-core 0.1.32. The master branch does not have the relevant optimization and is probably fine.

Platform

x86_64 Linux

Crates

tracing-core

Description

Under the right set of circumstances, it is possible for all events and spans to be silenced, regardless of the subscriber's configuration.

The particular sequence of events is as follows: 1) A Dispatch object is created, wrapping any kind of subscriber (an fmt subscriber with maximum logging will do). The object must not be set as the global or thread-local dispatcher yet. 2) A tracing macro is called. Since no dispatcher is configured yet, nothing is printed, as expected. 3) The dispatch object from step 1 is registered as a thread-local dispatcher, using tracing::dispatcher::with_default 4) The same line of code as step 2 is executed again. One would expect this to print, since a dispatcher is now configured, but instead nothing happens again.

The following piece of code demontrates the bug is a deterministic way. You can achieve a similar result using only the higher level tracing::subscriber::with_default API, but that requires multiple threads and a bit of luck to get the right order of operations.

fn is_enabled() -> bool {
    tracing::info!("log me maybe");
    return tracing::enabled!(tracing::Level::DEBUG);
}

fn main() {
    let subscriber = tracing_subscriber::fmt()
        .with_max_level(tracing::Level::TRACE)
        .finish();
    let dispatch = tracing::Dispatch::new(subscriber);

    // This returns false and doesn't print anything, as expected
    is_enabled();

    tracing::dispatcher::with_default(&dispatch, || {
        // This should have returned true, but does not.
        assert!(is_enabled());
    });
}

I've tracked the bug down to an optimization in tracing-core's callsite module, specifically the has_just_one atomic boolean.

The core of the issue comes from the has_just_one optimization, which is in contradiction with the fact that there are in fact two dispatchers: the global no-op one and the registered but not yet active one. One fix would be to register the global no-op to make the has_just_one flag more accurate. However I believe that by doing this, has_just_one would pretty much never be true. I think an easier solution would be to remove that optimization altogether. Indeed, it doesn't even exist on the master branch anymore.

If there had been other dispatchers around, has_just_one would have been false and the fallback code path would have worked fine. This can be tested by adding let _dont_drop_me = tracing::Dispatch::new(tracing::subscriber::NoSubscriber::new()); as the first line of the main function, to create a dummy dispatcher, enough to avoid that optimization.


This bug is probably not present on the master branch given that the optimization doesn't exist on it. I don't know what your policy is, but given that 0.1 is still the only published branch, it would be nice to have a bugfix for this issue. If you think dropping the has_just_one optimization is acceptable and reasonable I'll happily make a PR for it.