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

lazy_static "spin_no_std" feature is non-additive and can break unrelated code. #56

Open
eric-seppanen opened this issue Aug 18, 2023 · 5 comments

Comments

@eric-seppanen
Copy link

num-bigint-dig depends on lazy_static with features = ["spin_no_std"]. This can break other crates in the same workspace.

lazy_static's spin_no_std feature is non-additive; it causes lazy_static to replace use of std::sync::Once with spin:once::Once. This may seem like a harmless replacement, but:

  • spin::once::Once<T> has different trait bounds. Namely, it only implements Sync where T: Send + Sync while lazy_static using std::sync::Once only requires T: Sync.
  • This means that anyone using lazy_static on a non-Send type will see their code break if the spin_no_std feature is enabled.
  • Adding num-bigint-dig as a dependency to a large workspace means that every crate in that workspace now gets the modified lazy_static code using spin with stricter trait bounds.

This is currently happening to me: I added rsa to a large workspace, and that change causes compile errors in unrelated (previously working) code:

error[E0277]: `*const u8` cannot be sent between threads safely
    --> image.rs:25:1
     |
25   | / lazy_static::lazy_static! {
26   | |     pub(crate) static ref HELLO_IMAGE: Option<ImageBuffer> = {
27   | |         let image_bytes = std::fs::read("hello.png").ok()?;
28   | |
...    |
36   | |     };
37   | | }
     | |_^ `*const u8` cannot be sent between threads safely

I'm not sure how to handle this, but it would be nice if there were a feature in num-bigint-dig to disable this behavior. As it is, I'm unable to add an rsa dependency unless I fork+patch num-bigint-dig to disable that feature.

@tarcieri
Copy link

There's not a lot of uses of lazy_static. Maybe it could be migrated to once_cell now that it's been merged into core/std?

@dignifiedquire
Copy link
Owner

I like the idea switching to once_cell. I would just use the library version for now though, as the std version was only stabilized in 1.70

@dignifiedquire
Copy link
Owner

Once servo/rust-smallvec#301 is solved, we could get rid of it entirely

@eric-seppanen
Copy link
Author

Just to clarify, the trait bounds problem is not that significant; the more serious problem is that it forces everything in the workspace to use spinlocks instead of OS-level blocking for all lazy_static instances.

To paraphrase matklad, replacing an OS-level mutex with a spinlock is bad. If people want spinlocks for bare-metal environments, they should opt in to that approach (presumably by disabling default features). And this should not happen (by accident or omission) on targets with OS mutex support.

@tarcieri
Copy link

Perhaps we could start by disabling the spin_no_std feature?

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

No branches or pull requests

3 participants