From 86a84f821372e16b0ae1d1c4ca9be68951fefca0 Mon Sep 17 00:00:00 2001 From: Valentin Date: Tue, 23 Apr 2024 23:18:21 +0200 Subject: [PATCH] runtime: clarify misleading use of UnsafeCell::with_mut The code that we're removing calls UnsafeCell::with_mut with the argument `std::mem::drop`. This is misleading because the use of `drop` has no effect. `with_mut` takes an argument of type `impl FnOnce(*mut T) -> R`. The argument to the argument function is a pointer. Dropping a pointer has no effect. The comment above the first instance of this pattern claims that this releases some resource. This is false because the call has no effect. The intention might have been to drop the value behind the pointer. If this did happen, it would be a bug because the resource (`waker`) would be dropped again at the end of the function when the containing object is dropped. I looked through the history of this code. This code originally called `with_mut` with the argument `|_| ()`. Calling `with_mut` with an argument function that does nothing has a side effect when testing with loom. When testing with loom, the code uses loom's UnsafeCell type instead of std's. The intention of the code was likely to make use of that side effect because we expect to have exclusive access here as we are going to drop the containing object. The side effect is that loom checks that Rust's reference uniqueness properties are upheld. To continue to check this, I have only removed the use of `drop` while keeping `with_mut`. It would be even better to have loom check this implicitly when UnsafeCell is dropped. I created an issue about this in loom [1]. [1] https://github.com/tokio-rs/loom/issues/349 --- tokio/src/runtime/task/harness.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tokio/src/runtime/task/harness.rs b/tokio/src/runtime/task/harness.rs index cf19eea83bb..8479becd80a 100644 --- a/tokio/src/runtime/task/harness.rs +++ b/tokio/src/runtime/task/harness.rs @@ -249,11 +249,11 @@ where } pub(super) fn dealloc(self) { - // Release the join waker, if there is one. - self.trailer().waker.with_mut(drop); - - // Check causality - self.core().stage.with_mut(drop); + // Observe that we expect to have mutable access to these objects + // because we are going to drop them. This only matters when running + // under loom. + self.trailer().waker.with_mut(|_| ()); + self.core().stage.with_mut(|_| ()); // Safety: The caller of this method just transitioned our ref-count to // zero, so it is our responsibility to release the allocation.