tokio-rs / tracing

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

use `once_cell` instead of `static mut` #2958

Open LaoLittle opened 2 months ago

LaoLittle commented 2 months ago

Motivation

References to a static mut variable would become a hard error in rust 2024.

#[inline]
fn get_global() -> &'static Dispatch {
    if GLOBAL_INIT.load(Ordering::SeqCst) != INITIALIZED {
        return &NONE;
    }
    unsafe {
        // This is safe given the invariant that setting the global dispatcher
        // also sets `GLOBAL_INIT` to `INITIALIZED`.
        &GLOBAL_DISPATCH
    }
}

Link

Issue

Solution

Use once_cell::sync::OnceCell to do the one-time global initialization.

mladedav commented 2 months ago

In light of #2949, would it be possible to use std::sync::OnceLock instead?

LaoLittle commented 2 months ago

In light of #2949, would it be possible to use std::sync::OnceLock instead?

yeah, if we can use rust 1.70

mladedav commented 2 months ago

If we care about compiling on the 2024 edition the MSRV would be 1.82.

I mean it's nice to be ready and all but personally, I think this can be blocked until tracing uses 1.70 as this would be effectively a temporary change.

LaoLittle commented 2 months ago

I think this can be blocked until tracing uses 1.70 as this would be effectively a temporary change.

I'm ok with that

kaffarell commented 2 months ago

Hmm, IMO OnceCell is wrong, because it's not thread-safe (and static mut is thread safe if the inner type is Sync). We need to wait for OnceLock...

LaoLittle commented 2 months ago

Hmm, IMO OnceCell is wrong, because it's not thread-safe.

The OnceCell in this pr is once_cell::sync::OnceCell, which is thread-safe. It is true that OnceCell in std is not thread-safe, though.

and static mut is thread safe if the inner type is Sync

But I think thread-safety of Sync is only guaranteed with static (not static mut).

kaffarell commented 2 months ago

The OnceCell in this pr is once_cell::sync::OnceCell, which is thread-safe. It is true that OnceCell in std is not thread-safe, though.

Oops, though the std::cell::OnceCell was used, my bad :)

But I think thread-safety of Sync is only guaranteed with static (not static mut).

Yes, because with static mut you can mutate the inner type, losing the 'thread-safety' things you added in there :)

Edit: nevertheless this change looks good, lets get rid of the static mut's!