Code is unsound

[DzenanJupic]

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 (https://github.com/rust-lang/rust/issues/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.

[DzenanJupic]

Drawbacks of this new implementation

• The compiler now forces the Futures and T to be Sync + Send

[DzenanJupic]

I would also recommend yanking previous versions