Use Pin to pin RWLock. #77865
Use Pin to pin RWLock. #77865
Conversation
rust-highfive
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #82982) made this pull request unmergeable. Please resolve the merge conflicts. |
Dylan-DPC-zz
added
S-waiting-on-author
jackh726
added
the
T-libs
| /// Behavior is undefined if there are any currently active users of this | ||
| /// lock. | ||
| impl Drop for RWLock { | ||
| #[inline] | ||
| pub unsafe fn destroy(&self) { | ||
| self.0.destroy() | ||
| fn drop(&mut self) { | ||
| // SAFETY: The rwlock wasn't moved since using any of its | ||
| // functions, because they all require a Pin. |
There was a problem hiding this comment.
Isn't this changing the safety preconditions? Previously there was something about "active users", which is no longer mentioned.
This seems related to #85434. The "no active users" condition is not actually upheld by the users of this code. So it makes sense to remove this comment here, but IMO then there should be a comment one layer down -- because there pthread_mutex_destroy is being called and the safety precondition does not ensure all that needs to be ensured. (Basically, this moves the "active users" comment instead of removing it entirely, and acknowledges that we have a gap in our safety reasoning here.)
|
@m-ou-se any updates on this? |
|
Closing this due to inactivity. |
Use Pin to guarantee the no-moving requirement of
sys_common::RWLock.This makes some things safe that were unsafe before.
sys_common::RWLock::destroyis now changed to a regular (safe) Drop implementation, because it no longer has unsafe requirements.This change leaves the unsafe
sys::RwLockuntouched. Those implementations should probably also use aPinto make them a bit safer, and move theirdestroy()toDrop. But that can be a later change. (I want to wait for #77666 and #77657 before touchingsys.)r? @withoutboats
Following our conversation on Zulip, this is a better example where
Pin<&Self>is useful.