Skip to content

Commit

Permalink
runtime: remove misleading use of UnsafeCell::with_mut
Browse files Browse the repository at this point in the history
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.

Instead of removing the whole code, I considered changing `drop` back to
`|_|()` and documenting what I wrote above. I decided on removing the
code because nowhere else do we use `with_mut` in this way and the
purpose of this check would be better served in loom directly as part of
UnsafeCell's Drop implementation. I created an issue about this in loom
[1].

[1] tokio-rs/loom#349
  • Loading branch information
e00E committed Apr 23, 2024
1 parent 1961890 commit 31f43bf
Showing 1 changed file with 0 additions and 6 deletions.
6 changes: 0 additions & 6 deletions tokio/src/runtime/task/harness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,12 +249,6 @@ 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);

// Safety: The caller of this method just transitioned our ref-count to
// zero, so it is our responsibility to release the allocation.
//
Expand Down

0 comments on commit 31f43bf

Please sign in to comment.