Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: remove misleading use of UnsafeCell::with_mut #6513

Merged
merged 1 commit into from Apr 25, 2024

Conversation

e00E
Copy link
Contributor

@e00E e00E commented Apr 23, 2024

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

@github-actions github-actions bot added R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR labels Apr 23, 2024
@mox692 mox692 added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Apr 24, 2024
@Darksonn
Copy link
Contributor

Thank you for the PR. This suggestion makes a lot of sense.

I would like to have this fixed in loom before we merge this. In the meantime, we can merge a PR that fixes the incorrect comments if you would like.

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] tokio-rs/loom#349
@e00E
Copy link
Contributor Author

e00E commented Apr 24, 2024

I've changed the PR to only remove the use of drop while keeping with_mut. The last paragraph of the commit message is now:

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].

@Darksonn Darksonn merged commit 731dde2 into tokio-rs:master Apr 25, 2024
82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants