[refactor] replace lazy_static with LazyLock #5055
Conversation
| @@ -39,7 +38,7 @@ tikv-jemallocator = {workspace = true} | |||
| [dev-dependencies] | |||
| ring = "0.16.19" | |||
| warp = "0.3.5" | |||
| tokio = "1.15" | |||
| tokio = { version = "1.15", features = ["rt-multi-thread"] } | |||
There was a problem hiding this comment.
Runtime::new() only exists when this feature is enabled, the tests wouldn't compile for me otherwise, hence why I added the rt-multi-thread feature.
| .expect("Failed to create COMMANDS_PROCESSED") | ||
| }); | ||
|
|
||
| pub static DGK_VOTES_SUBMITTED: LazyLock<IntCounter> = LazyLock::new(|| { |
There was a problem hiding this comment.
I assume this is supposed to be DKG and not DGK? If so, it's also incorrect in the metric name.
There was a problem hiding this comment.
Yes, definitely. I think it's fine to go ahead and fix that in this PR if you want.
cylewitruk
changed the title
|
Don't forget to update |
…static-with-lazy-lock' into chore/replace-lazy-static-with-lazy-lock
|
@jbencin I think all of your comments are resolved now. Really like I went a step further and reverted to I added |
| @@ -1,5 +1,6 @@ | |||
| [workspace] | |||
| resolver = "2" | |||
| package.rust-version = "1.80" | |||
There was a problem hiding this comment.
I don't know about using a workspace-level MSRV... it's fine to require latest version for stacks-node, but we might want to be more conservative with Clarity, stacks-common and stackslib, as those are used by other projects
There was a problem hiding this comment.
I thought about that too, but now when clarity, stacks-common and stackslib all use LazyCell and/or LazyLock, they need to be at 1.80 in any case. We can keep the rust-version flag local to the packages if that's preferred, I just figured it's easier to keep the whole project at an announced MSRV 🤷 No strong opinions here.
There was a problem hiding this comment.
While it's technically correct because the MSRV is 1.80 for all packages right now, and even though it can be overriden in package-level Cargo.toml files, this implies that we have some global MSRV policy which applies to all packages, which I don't think we want. So IMO it's better not to do this, but I don't really have strong feelings on it, and I'd approve either way
There was a problem hiding this comment.
Let's see if anyone else has any opinions on the matter -- otherwise it's just a couple of minutes to revert
There was a problem hiding this comment.
@jbencin @obycode who need to weigh in here? Should the ClarityWASM folks and Clarinet folks weigh in? cc @hugocaillard
There was a problem hiding this comment.
@jcnelson Hugo's on vacation right now, but I checked and Clarinet builds on Rust 1.80 without warnings. There are commit messages like this in the history:
445ea332 (4 weeks ago) Hugo C: fix: clippy warning v1.80 (#1518)
So this PR shouldn't be a problem for them
There was a problem hiding this comment.
Alright, it's fine by me as long as its easy to change back if someone complains.
| .expect("Failed to create COMMANDS_PROCESSED") | ||
| }); | ||
|
|
||
| pub static DGK_VOTES_SUBMITTED: LazyLock<IntCounter> = LazyLock::new(|| { |
There was a problem hiding this comment.
Yes, definitely. I think it's fine to go ahead and fix that in this PR if you want.
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "8a30b2e23b9e17a9f90641c7ab1549cd9b44f296d3ccbf309d2863cfe398a0cb" | ||
| checksum = "6e4503c46a5c0c7844e948c9a4d6acd9f50cccb4de1c48eb9e291ea17470c678" |
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" | ||
| checksum = "bbd2bcb4c963f2ddae06a2efc7e9f3591312473c50c6685e1f298068316e66fe" |
There was a problem hiding this comment.
I thought we were dropping this?
There was a problem hiding this comment.
It's still used by some of our dependencies though, like prometheus
Resolves #5035 however using
std::sync::LazyLockinstead ofstd::cell::LazyCellbecause most of the values are used as statics (LazyCellis forconst-like behavior). Also changed a bunch of.unwrap()s to.expect()s to improve eventual error interpretation.const+LazyCellcould likely be used for a handful of the values, but to determine those the usage of each needs to be evaluated, so I opted forLazyLockfor all of them as it most closely mimicsstatic_lazy!.