hjiayz / async_once Goto Github PK

Addition of jwt-simple to Cargo.toml breaks compilation of AsyncOnce blocks

error[E0277]: `*const u32` cannot be sent between threads safely
  --> src/main.rs:4:1
   |
4  | / lazy_static! {
5  | |     static ref FOO: AsyncOnce<u32> = AsyncOnce::new(
6  | |         async {
7  | |             1
...  |
13 | |     static ref BAR: u32 = 2;
14 | | }
   | |_^ `*const u32` cannot be sent between threads safely
   |
   = help: the trait `Send` is not implemented for `*const u32`
   = note: required because of the requirements on the impl of `Send` for `Cell<*const u32>`
   = note: required because it appears within the type `AsyncOnce<u32>`
   = note: required because of the requirements on the impl of `Sync` for `spin::once::Once<AsyncOnce<u32>>`
   = note: required because it appears within the type `lazy_static::lazy::Lazy<AsyncOnce<u32>>`
   = note: shared static variables must have a type that implements `Sync`
   = note: this error originates in the macro `__lazy_static_create` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: `(dyn Future<Output = u32> + 'static)` cannot be sent between threads safely
  --> src/main.rs:4:1
   |
4  | / lazy_static! {
5  | |     static ref FOO: AsyncOnce<u32> = AsyncOnce::new(
6  | |         async {
7  | |             1
...  |
13 | |     static ref BAR: u32 = 2;
14 | | }
   | |_^ `(dyn Future<Output = u32> + 'static)` cannot be sent between threads safely
   |
   = help: the trait `Send` is not implemented for `(dyn Future<Output = u32> + 'static)`
   = note: required because of the requirements on the impl of `Send` for `Unique<(dyn Future<Output = u32> + 'static)>`
   = note: required because it appears within the type `Box<(dyn Future<Output = u32> + 'static)>`
   = note: required because it appears within the type `Pin<Box<(dyn Future<Output = u32> + 'static)>>`
   = note: required because it appears within the type `Result<u32, Pin<Box<(dyn Future<Output = u32> + 'static)>>>`
   = note: required because of the requirements on the impl of `Send` for `Mutex<Result<u32, Pin<Box<(dyn Future<Output = u32> + 'static)>>>>`
   = note: required because it appears within the type `AsyncOnce<u32>`
   = note: required because of the requirements on the impl of `Sync` for `spin::once::Once<AsyncOnce<u32>>`
   = note: required because it appears within the type `lazy_static::lazy::Lazy<AsyncOnce<u32>>`
   = note: shared static variables must have a type that implements `Sync`
   = note: this error originates in the macro `__lazy_static_create` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `async_once_jwt_simple` due to 2 previous errors

I honestly wish i was joking and I understood why.

Minimal reproducible example below:
https://github.com/Charles-Schleich/async_once_jwt_simple

No work around or fix so far

add dependency sea-orm and then not work

I am not certain if this is a legitimate problem since I haven't written any breaking tests but looking at the code it seems like it is possible to return a Poll::Pending without registering a waker to fire when the lock was not ready but becomes ready later.

I stumbled over this crate and it seems quite useful. Unfortunately, the code is unsound and also a bit messy. I'd like to help here since this crate could be quite useful for quite a few people.

Only creating the PR would solve the problem but I'd also like to explain why this code is unsound and how to fix it.

Unsoundness

There's quite a lot going on and I didn't work my way through the complete code base, so there might be something I didn't spot. There are mostly two things I spotted:

Unconditional Sync implementation

lib.rs:46

unsafe impl<T: 'static> Sync for AsyncOnce<T> {}

This implementation of Sync is incorrect for two reasons: First T itself might not be Sync, in which case the AsyncOnce can also not be Sync. And second AsyncOnce contains a std::cell::Cell<*const T>. std::cell::Cell is !Sync since it is not thread-safe. So all access to it has to be synchronized somehow, which does not happen.

Race condition

lib.rs:85-91

impl<T> Future for &'static AsyncOnce<T> {
    type Output = &'static T;
    #[inline(always)]
    fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<&'static T> {
        if let Some(ptr) = unsafe { self.ptr.get().as_ref() } {
            return Poll::Ready(ptr);
        }

        // -- snip --
    }
}

std::cell::Cell is not thread-safe - as already mentioned above. But due to the implementation of Sync for AsyncOnce <AsyncOnce as Future>::poll might be called from multiple threads (The exclusive reference in Pin<&mut Self> does not help in this case, since Future is implemented for &'static AsyncOnce). The self.ptr.get() in line 89 in combination with self.ptr.set(ptr); in line 122 leads to a race condition.

Fixing the problems

There are three main ways of fixing the code:

1. Implementing a custom once_cell::sync::OnceCell like structure
2. Slapping once_cell::sync::OnceCell into AsynOnce and keeping everything else the same
3. Slapping once_cell::sync::OnceCell into AsyncOnce and reimplementing the interface

once_cell, in case you don't already know, is a crate that provides one-time initialization primitives. It's also in the process of being merged into std (rust-lang/rust#74465).

• The first approach is quite a bit of boilerplate and just unnecessary.
• The second approach, which I went for in my PR, is the least effort and keeps the interface mostly the same.
• In the long run, I would recommend the third approach since it gives you more flexibility in what you put into AsyncOnce and how you can initialize it. I first tried to go this way but was too lazy to adapt all the tests 😄. I can explain the approach I would take if you're interested in it.

By just putting an once_cell::sync::OnceCell into AsyncOnce you solve all the soundness issues and also make the code a lot simpler.
Besides that, I also replaced std::sync::Mutex with parking_lot::Mutex, since it's the general/official recommendation.