Add OnceCell #3591

b-naber · 2021-03-08T10:36:56Z

Separating #3513 into two PRs for clarity, as requested by @Darksonn. Since Lazy depends on OnceCell, a PR for Lazy will be opened once this PR lands.

tokio/src/sync/once_cell.rs

b-naber · 2021-03-12T21:42:56Z

@Darksonn Thanks for the review. I addressed your comments.

tokio/src/sync/once_cell.rs

b-naber · 2021-03-17T17:38:56Z

@Darksonn Not sure about what to do about the clippy errors in the async_send_sync tests. I tried to use type aliases, but get 'never used' warnings for some reason.

Edit: In the previous commit I got errors in the clippy pass because the type for the function pointer was 'too complex' in the ?async_send_sync tests. The suggestion was to use type definitions, but apparently this is not the right way to handle this either.

Darksonn · 2021-03-20T09:02:44Z

Feel free to silence "too complex" errors in the async_send_sync.rs file. You can add an #![allow(whatever_the_lint_name_is)] at the top to do so.

b-naber · 2021-03-21T23:35:10Z

@Darksonn CI passes now.

Darksonn · 2021-03-22T08:20:39Z

tokio/src/sync/once_cell.rs

+    /// Moves the value out of the cell and drops the cell afterwards.
+    pub fn into_inner(self) -> Result<T, NotInitializedError> {
+        if self.initialized() {
+            Ok(unsafe { self.value.with(|ptr| ptr::read(ptr).assume_init()) })
+        } else {
+            Err(NotInitializedError(()))
+        }
+    }
+
+    /// Takes ownership of the current value, leaving the cell unitialized.
+    pub fn take(&mut self) -> Result<T, NotInitializedError> {
+        if self.initialized() {
+            // Note: ptr::read does not move the value out of `self.value`. We need to use


Should these also return Option for consistency with get?

Darksonn · 2021-03-22T08:27:40Z

tokio/src/sync/once_cell.rs

+    pub fn take(&mut self) -> Result<T, NotInitializedError> {
+        if self.initialized() {
+            // Note: ptr::read does not move the value out of `self.value`. We need to use
+            // `self.initialized` to prevent incorrect accesses to value
+            let value = unsafe { self.value.with(|ptr| ptr::read(ptr).assume_init()) };
+            *self.value_set.get_mut() = false;
+            Ok(value)
+        } else {


This still leaves the semaphore closed. Consider changing the implementation to:

let old_me = std::mem::replace(self, OnceCell::new()); old_me.into_inner()

Darksonn · 2021-03-22T08:31:55Z

tokio/src/sync/once_cell.rs

+        let new_cell = OnceCell::new();
+        if let Some(value) = self.get() {
+            match new_cell.set(value.clone()) {
+                Ok(()) => (),
+                Err(_) => unreachable!(),
+            }
+        }
+        new_cell


This is kinda ugly. Should we have a constructor that takes an Option<T> and returns an already initialized cell if the option is some?

Yes, I think that's a good idea.

What would be a good name for that? new_with maybe?

I think new_with is an ok name.

b-naber · 2021-03-22T12:58:51Z

@Darksonn addressed your comments.

Darksonn · 2021-03-22T12:59:24Z

tokio/src/sync/once_cell.rs

+
+impl<T: Clone> Clone for OnceCell<T> {
+    fn clone(&self) -> OnceCell<T> {
+        OnceCell::new_with(self.get().map(|v| (*v).clone()))


You can use Option::cloned here.

Suggested change

OnceCell::new_with(self.get().map(|v| (*v).clone()))

OnceCell::new_with(self.get().cloned())

Darksonn · 2021-03-22T13:03:13Z

tokio/src/sync/once_cell.rs

+        let (value_set, value) = if let Some(v) = value {
+            (AtomicBool::new(true), UnsafeCell::new(MaybeUninit::new(v)))
+        } else {
+            (
+                AtomicBool::new(false),
+                UnsafeCell::new(MaybeUninit::uninit()),
+            )
+        };
+        OnceCell {
+            value_set,
+            value,
+            semaphore: Semaphore::new(1),
+        }


You need to close the semaphore in the case where the value is set.

Suggested change

let (value_set, value) = if let Some(v) = value {

(AtomicBool::new(true), UnsafeCell::new(MaybeUninit::new(v)))

} else {

(

AtomicBool::new(false),

UnsafeCell::new(MaybeUninit::uninit()),

)

};

OnceCell {

value_set,

value,

semaphore: Semaphore::new(1),

}

if let Some(v) = value {

let semaphore = Semaphore::new(0);

semaphore.close();

OnceCell {

value_set: AtomicBool::new(true),

value: UnsafeCell::new(MaybeUninit::new(v)),

semaphore,

}

} else {

OnceCell::new()

}

I agree that this is more consistent, but it should't matter in practice since we can only acquire the permit when value_set is true, which we set to false if value is Some.

I do want the semaphore to be closed here, because it makes the safety arguments you have elsewhere easier to follow if the semaphore is always closed when the value is set.

Darksonn · 2021-03-22T13:06:31Z

tokio/src/sync/once_cell.rs

+    pub fn into_inner(self) -> Option<T> {
+        if self.initialized() {
+            Some(unsafe { self.value.with(|ptr| ptr::read(ptr).assume_init()) })
+        } else {
+            None
+        }
+    }


Can you please add a test that:

The destructor of the T inside is called when the cell is dropped normally.

The destructor of the T inside is not called when into_inner is used.

How do you check whether the destructor was called in rust?

I guess just implementing a new type including a drop implementation?

You can define a global counter that you increment in a custom destructor.

static NUM_DROPS: AtomicU32 = AtomicU32::new(0);

Make sure to use different counters for each test. They may run in parallel.

Darksonn · 2021-03-22T13:06:58Z

tokio/src/sync/once_cell.rs

+        if self.initialized() {
+            let old_me = std::mem::replace(self, OnceCell::new());
+            old_me.into_inner()
+        } else {
+            None
+        }


This can be simplified.

Suggested change

if self.initialized() {

let old_me = std::mem::replace(self, OnceCell::new());

old_me.into_inner()

} else {

None

}

let old_me = std::mem::replace(self, OnceCell::new());

old_me.into_inner()

mem::take too

Wow, that makes it even simpler.

Suggested change

if self.initialized() {

let old_me = std::mem::replace(self, OnceCell::new());

old_me.into_inner()

} else {

None

}

std::mem::take(self).into_inner()

b-naber · 2021-03-24T19:26:58Z

@Darksonn So the new tests don't work for some reason. When OnceCell is dropped the drop method of its value is not called and into_inner does call drop. I don't understand why this happens.

edit: actually into_inner doesn't call drop. I didn't know that assigning to a _ is equivalent to dropping the assigned value.

edit2: MaybeUninit apparently never calls drop on its value, so we need a drop implementation for OnceCell, don't we?

Darksonn · 2021-03-24T21:53:07Z

tokio/tests/sync_once_cell.rs

+    let _ = once_cell.into_inner();
+    let count = NUM_DROPS.load(Ordering::Acquire);
+    assert!(count == 0);


Assigning to underscore runs the destructor.

Suggested change

let _ = once_cell.into_inner();

let count = NUM_DROPS.load(Ordering::Acquire);

assert!(count == 0);

let fooer = once_cell.into_inner();

let count = NUM_DROPS.load(Ordering::Acquire);

assert!(count == 0);

drop(fooer);

let count = NUM_DROPS.load(Ordering::Acquire);

assert!(count == 1);

Darksonn · 2021-03-24T21:55:52Z

We're going to need a destructor, yes.

b-naber · 2021-03-24T23:34:02Z

@Darksonn can you take another look, please?

Darksonn

I don't have any further major comments.

Darksonn · 2021-03-25T08:12:36Z

tokio/src/sync/once_cell.rs

+    pub fn into_inner(self) -> Option<T> {
+        if self.initialized() {
+            // Set to uninitialized for the destructor of `OnceCell` to work properly
+            self.value_set.store(false, Ordering::Release);


Whenever you are in self or &mut self methods, you can access it like this:

Suggested change

self.value_set.store(false, Ordering::Release);

*self.value_set.get_mut() = false;

Doing it in this manner is cheaper because it doesn't involve atomic operations.

Darksonn · 2021-03-25T08:14:59Z

tokio/tests/sync_once_cell.rs

+    let _v = once_cell.into_inner();
+    let count = NUM_DROPS.load(Ordering::Acquire);
+    assert!(count == 0);


Suggested change

let _v = once_cell.into_inner();

let count = NUM_DROPS.load(Ordering::Acquire);

assert!(count == 0);

let value = once_cell.into_inner();

let count = NUM_DROPS.load(Ordering::Acquire);

assert!(count == 0);

drop(value);

let count = NUM_DROPS.load(Ordering::Acquire);

assert!(count == 1);

b-naber · 2021-03-25T14:49:30Z

I get an error in CI unrelated to this PR:

error: panic message is not a string literal
   --> tokio/src/time/driver/entry.rs:547:20
    |
547 |             panic!(crate::util::error::RUNTIME_SHUTTING_DOWN_ERROR);
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D non-fmt-panic` implied by `-D warnings`
    = note: this is no longer accepted in Rust 2021
help: add a "{}" format string to Display the message
    |
547 |             panic!("{}", crate::util::error::RUNTIME_SHUTTING_DOWN_ERROR);
    |                    ^^^^^
help: or use std::panic::panic_any instead
    |
547 |             std::panic::panic_any(crate::util::error::RUNTIME_SHUTTING_DOWN_ERROR);
    |             ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

error: could not compile `tokio`

Darksonn · 2021-03-25T16:13:12Z

That must be a new warning with the new version of Rust. You can just apply its first suggestion and it should be ok.

Darksonn · 2021-03-26T09:07:51Z

Once #3647 is merged, you can fix the CI errors by merging master.

b-naber · 2021-03-27T08:17:25Z

@Darksonn CI passes now. Thanks for the review and the many suggestions, those were quite helpful.

Darksonn

The implementation looks correct. I only have comments on documentation and tests left.

Great work!

Darksonn · 2021-03-27T10:32:47Z

tokio/src/sync/once_cell.rs

+///     let result1 = tokio::spawn(async {
+///         ONCE.get_or_init(some_computation).await
+///     }).await.unwrap();


This makes the example a bit simpler.

Suggested change

/// let result1 = tokio::spawn(async {

/// ONCE.get_or_init(some_computation).await

/// }).await.unwrap();

/// let result1 = ONCE.get_or_init(some_computation).await;

Darksonn · 2021-03-27T10:40:35Z

tokio/src/sync/once_cell.rs

+        }
+    }
+
+    /// Moves the value out of the cell and drops the cell afterwards.


Suggested change

/// Moves the value out of the cell and drops the cell afterwards.

/// Moves the value out of the cell, destroying the cell in the process.

Darksonn · 2021-03-27T10:43:02Z

tokio/src/sync/once_cell.rs

+impl<T: fmt::Debug> Error for SetError<T> {}
+
+impl<T> SetError<T> {
+    /// Whether `SetError` is `SetError::AlreadyInitializEderror`


There's a typo here. Also, please put a period at the end (also for the other method).

Darksonn · 2021-03-27T10:44:58Z

tokio/tests/sync_once_cell.rs

+    static ONCE: OnceCell<u32> = OnceCell::const_new();
+
+    rt.block_on(async {
+        time::pause();


Use start_paused instead.

Darksonn · 2021-03-27T10:46:38Z

tokio/tests/sync_once_cell.rs

+
+    {
+        let once_cell = OnceCell::new();
+        let _ = once_cell.set(fooer);


Can you assert that this set succeeds?

tokio/tests/sync_once_cell.rs

albel727 · 2021-03-28T12:48:13Z

How about a method for conditional initialization? Sometimes one wants to repeat attempts at initializing until something comes along. The following should pass errors through and leave the cell uninitialized, storing only the first Ok result instead.

    pub async fn get_or_try_init<E, F, Fut>(&self, f: F) -> Result<&T, E>
    where
        F: FnOnce() -> Fut,
        Fut: Future<Output = Result<T, E>>,
    {
        if self.initialized() {
            // SAFETY: once the value is initialized, no mutable references are given out, so
            // we can give out arbitrarily many immutable references
            unsafe { Ok(self.get_unchecked()) }
        } else {
            // After acquire().await we have either acquired a permit while self.value
            // is still uninitialized, or current thread is awoken after another thread
            // has intialized the value and closed the semaphore, in which case self.initialized
            // is true and we don't set the value here
            match self.semaphore.acquire().await {
                Ok(_permit) => {
                    if !self.initialized() {
                        // If `f()` panics or `select!` is called, this `get_or_try_init` call
                        // is aborted and the semaphore permit is dropped.
                        let value = f().await;

                        match value {
                            Ok(value) => {
                                // SAFETY: There is only one permit on the semaphore, hence only one
                                // mutable reference is created
                                unsafe { self.set_value(value) };

                                // SAFETY: once the value is initialized, no mutable references are given out, so
                                // we can give out arbitrarily many immutable references
                                unsafe { Ok(self.get_unchecked()) }
                            },
                            Err(e) => Err(e),
                        }
                    } else {
                        unreachable!("acquired semaphore after value was already initialized.");
                    }
                }
                Err(_) => {
                    if self.initialized() {
                        // SAFETY: once the value is initialized, no mutable references are given out, so
                        // we can give out arbitrarily many immutable references
                        unsafe { Ok(self.get_unchecked()) }
                    } else {
                        unreachable!(
                            "Semaphore closed, but the OnceCell has not been initialized."
                        );
                    }
                }
            }
        }
    }

Darksonn · 2021-03-28T14:09:47Z

@albel727 Yeah, that would be a reasonable addition as well.

b-naber · 2021-03-31T20:37:40Z

@Darksonn Updated the PR. Do you want @albel727's suggested method to be included in this PR?

Darksonn · 2021-03-31T20:44:51Z

It is up to you if you want to do it in this PR.

b-naber · 2021-04-01T20:31:33Z

@Darksonn Added that method and a test for it. Can you take another look, please?

Darksonn

Thanks for all your work. It looks good. 👍

Darksonn · 2021-04-01T20:56:18Z

tokio/tests/sync_once_cell.rs

+    }
+
+    let once_cell = OnceCell::new();
+    let _ = once_cell.set(fooer);


Please assert that the set succeeds.

Suggested change

let _ = once_cell.set(fooer);

assert!(once_cell.set(fooer).is_ok());

Darksonn · 2021-04-01T20:56:27Z

tokio/tests/sync_once_cell.rs

+    let fooer = once_cell.into_inner();
+    let count = NUM_DROPS.load(Ordering::Acquire);
+    assert!(count == 0);
+    mem::drop(fooer);


Suggested change

mem::drop(fooer);

drop(fooer);

b-naber · 2021-04-02T09:12:02Z

@Darksonn A test not related to this PR fails (tcp_into_std), why is that?

Darksonn · 2021-04-02T09:36:44Z

It failed with "Address already in use". I'll try rerunning the tests.

b-naber · 2021-04-03T15:01:53Z

@Darksonn thanks for re-running the test. Is there something that still needs to be done before this can be merged?

Darksonn

As a last sanity check, I always like to view the generated documentation and in doing so I found the following. Please also take a look at the generated doc yourself. Besides that, I think this is ready to be merged.

Darksonn · 2021-04-04T07:37:09Z

tokio/src/sync/once_cell.rs

+    /// If the value of the OnceCell was already set prior to this call
+    /// then [`SetError::AlreadyInitializedError`] is returned. If another thread
+    /// is initializing the cell while this method is called,
+    /// ['SetError::InitializingError`] is returned. In order to wait


Suggested change

/// ['SetError::InitializingError`] is returned. In order to wait

/// [`SetError::InitializingError`] is returned. In order to wait

b-naber · 2021-04-05T18:58:32Z

@Darksonn Thanks for catching that error. I couldn't find any further errors in the docs myself.

Darksonn · 2021-04-05T20:04:18Z

Thanks for all your work on this feature! 🎉

b-naber · 2021-04-05T20:21:44Z

Thanks for all your help.

Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Mar 8, 2021

Darksonn reviewed Mar 8, 2021

View reviewed changes

b-naber force-pushed the once_cell branch from 8d131c8 to dd83217 Compare March 12, 2021 22:01

Darksonn reviewed Mar 13, 2021

View reviewed changes

b-naber force-pushed the once_cell branch 3 times, most recently from ed48b81 to b6c6d99 Compare March 21, 2021 21:43

Darksonn reviewed Mar 22, 2021

View reviewed changes

b-naber force-pushed the once_cell branch from 8dd59b5 to 8dfc18d Compare March 22, 2021 11:54

Darksonn reviewed Mar 22, 2021

View reviewed changes

Darksonn reviewed Mar 24, 2021

View reviewed changes

Darksonn reviewed Mar 25, 2021

View reviewed changes

b-naber force-pushed the once_cell branch 2 times, most recently from 78415d1 to 15351a4 Compare March 25, 2021 14:40

b-naber added 3 commits March 26, 2021 20:17

add OnceCell

b3e521d

add and update tests

440a547

address review

fdadf94

add destructor

7b1be1a

b-naber force-pushed the once_cell branch from 7d09b5a to 33fce8e Compare March 26, 2021 19:21

suggestions

741b508

b-naber force-pushed the once_cell branch from 33fce8e to 741b508 Compare March 26, 2021 20:08

Darksonn reviewed Mar 27, 2021

View reviewed changes

suggestions

cdc3edc

add get_or_try_init

a92ce09

b-naber force-pushed the once_cell branch from 9b6d749 to a92ce09 Compare April 1, 2021 20:30

Darksonn reviewed Apr 1, 2021

View reviewed changes

suggestions

b58dda5

Darksonn approved these changes Apr 2, 2021

View reviewed changes

Darksonn reviewed Apr 4, 2021

View reviewed changes

fix docs

201cbf1

Darksonn merged commit f6e4e85 into tokio-rs:master Apr 5, 2021

b-naber deleted the once_cell branch April 5, 2021 20:21

Darksonn mentioned this pull request Apr 12, 2021

Prepare Tokio v1.5.0 #3695

Merged

Darksonn mentioned this pull request May 5, 2021

Add OnceCell and Lazy #3513

Closed

Darksonn mentioned this pull request Aug 17, 2021

Add an async OnceCell and Lazy #3482

Closed

	OnceCell::new_with(self.get().map(\|v\| (*v).clone()))
	OnceCell::new_with(self.get().cloned())

	self.value_set.store(false, Ordering::Release);
	*self.value_set.get_mut() = false;

	/// Moves the value out of the cell and drops the cell afterwards.
	/// Moves the value out of the cell, destroying the cell in the process.

	let _ = once_cell.set(fooer);
	assert!(once_cell.set(fooer).is_ok());

	/// ['SetError::InitializingError`] is returned. In order to wait
	/// [`SetError::InitializingError`] is returned. In order to wait

Add OnceCell #3591

Add OnceCell #3591

Conversation

b-naber commented Mar 8, 2021

b-naber commented Mar 12, 2021

b-naber commented Mar 17, 2021 • edited

Darksonn commented Mar 20, 2021

b-naber commented Mar 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Mar 24, 2021 • edited

Choose a reason for hiding this comment

Darksonn commented Mar 24, 2021

b-naber commented Mar 24, 2021

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Mar 25, 2021

Darksonn commented Mar 25, 2021

Darksonn commented Mar 26, 2021

b-naber commented Mar 27, 2021

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albel727 commented Mar 28, 2021 • edited

Darksonn commented Mar 28, 2021

b-naber commented Mar 31, 2021

Darksonn commented Mar 31, 2021

b-naber commented Apr 1, 2021

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Apr 2, 2021

Darksonn commented Apr 2, 2021

b-naber commented Apr 3, 2021

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Apr 5, 2021

Darksonn commented Apr 5, 2021

b-naber commented Apr 5, 2021

b-naber commented Mar 17, 2021 •

edited

b-naber commented Mar 24, 2021 •

edited

albel727 commented Mar 28, 2021 •

edited