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

use once_cell instead of static mut #2958

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LaoLittle
Copy link

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
Copy link
Contributor

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

@LaoLittle
Copy link
Author

LaoLittle commented Apr 28, 2024

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

yeah, if we can use rust 1.70

@mladedav
Copy link
Contributor

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
Copy link
Author

LaoLittle commented Apr 28, 2024

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
Copy link
Contributor

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
Copy link
Author

LaoLittle commented May 1, 2024

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
Copy link
Contributor

kaffarell commented May 2, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants