-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
sync: Implement map methods of parking_lot fame #2445
Conversation
ae9d6ef
to
dd7198b
Compare
May also want to note that this closes #2471 as well :) |
tokio/src/sync/rwlock.rs
Outdated
where | ||
F: FnOnce(&T) -> &U, | ||
{ | ||
let data = f(unsafe { &*self.lock.c.get() }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary for this to be unsafe
? Can we just dereference the guard normally here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@udoprog I believe you can probably remove the usages of unsafe from several other places too using the same technique. You still have several map and try_map functions that use unsafe in the same way you were before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @udoprog! I am the author of issue #2471 and PR #2472. Thank you so much for your work on this PR. Using your work as a reference made creating my PR much easier!
As a result of doing that PR, I ended up having a very close look at this one and noticed a few things that should probably be addressed. I hope you don't mind that I left a few comments.
Thanks again! Looking forward to this landing. :)
tokio/src/sync/rwlock.rs
Outdated
/// # } | ||
/// ``` | ||
#[inline] | ||
pub fn map<U, F>(self, f: F) -> MappedRwLockReadGuard<'a, U> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The map
/try_map
methods should probably be associated functions (using e.g. this: Self
) instead of methods that take self
. This is important to avoid conflicts with methods on the dereferenced type T
. The map
methods on parking_lot
are associated functions too.
(In fact, your documentation on this method even says "This is an associated function".)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, are you planning on adding map
and try_map
associated functions on MappedRwLockReadGuard
and MappedRwLockWriteGuard
? Without that it will only be possible to map the data once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointer. My head just refactored the methods into self
without realizing why.
Also, are you planning on adding
map
andtry_map
associated functions onMappedRwLockReadGuard
andMappedRwLockWriteGuard
? Without that it will only be possible to map the data once.
Not at the moment since I personally have no use for it. Feel free to add it in a follow up to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was proof-reading documentation and decided to add associated methods for the mapped variants. Please look them over. Thanks.
tokio/src/sync/rwlock.rs
Outdated
// NB: These impls need to be explicit since we're storing a raw pointer. | ||
// Safety: Stores a raw pointer to `T`, so if `T` is `Sync`, so is the lock | ||
// guard over `T`. | ||
unsafe impl<'a, T> Send for MappedRwLockReadGuard<'a, T> where T: Sync {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be implementing Sync
, not Send
? I don't know for sure, but it's suspicious given the code around it and the comment above it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified the comment. The impl is indeed supposed to be Send
if we want the guard to be sent across .await
for Send
futures. See if it makes sense to you now.
89b46b8
to
b4d2d25
Compare
@udoprog I managed to merge |
@sunjay Yeah, for sure. I just want to make sure that if we want to support upgradeable/downgradeable locks at some point we're not subject to soundness issues as commented here and in the documentation for the guard. I haven't wrapped my head around that issue yet though, so just mimicking the API for now. |
Eh. So the downgrade API for It is being kept though for |
Bumped to latest changes in Tokio. I still need this, so would like to somehow move it forward. If someone wants to see how it's used: https://github.com/udoprog/OxidizeBot/search?q=try_map&unscoped_q=try_map |
With regards to supporting upgrade/downgrade for guards: I'm not sure how much sense this would make with the current implementation of the Upgrading a Given that, IMO, I think I'd prefer to merge the change that presents a smaller API surface for now. If we end up changing our minds about this, we'll have the chance to break the API in 0.3, so we could change the map functions to return a different guard type then. It would be nice to see if @carllerche agrees, though! |
I'm fine with either at this point. Removed the other guard types for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me --- sorry for the delay on the review.
51bdc6f
to
3812179
Compare
Yeah. I'm pretty used to rebasing now. |
Generally, this mimics the way `MappedRwLock*Guard`s are implemented in `parking_lot`. By storing a raw pointer in the guards themselves referencing the mapped data and maintaining type invariants through `PhantomData`. I didn't try to think too much about this, so if someone has objections I'd love to hear them. I've also dropped the internal use of `ReleasingPermit`, since it made the guards unecessarily large. The number of permits that need to be released are already known by the guards themselves, and is instead governed directly in the relevant `Drop` impls. This has the benefit of making the guards as small as possible, for the non-mapped variants this means a single reference is enough. `fmt::Debug` impls have been adjusted to behave exactly like the delegating impls in `parking_lot`. `fmt::Display` impls have been added for all guard types which behave the same. This does change the format of debug impls, for which I'm not sure if we provide any guarantees.
Rebased. @carllerche I think everyone is waiting for an OK from someone because of the API addition, and I think that's you? But not sure. Could you have a look? |
I've introduced
MappedRwLockReadGuard
andMappedRwLockWriteGuard
, which are produced by the relevantmap
andtry_map
methods.Generally, this mimics the way
MappedRwLock*Guard
s are implemented inparking_lot
. By storing a raw pointer in the guards themselves referencing the mapped data and maintaining type invariants throughPhantomData
. I didn't try to think too much about this, so if someone has objections I'd love to hear them.I've also dropped the internal use of
ReleasingPermit
, since it made the guards unecessarily large. The number of permits that need to be released are already known by the guards themselves, and is instead governed directly in the relevantDrop
impls. This has the benefit of making the guards as small as possible, for the non-mapped variants this means a single reference is enough.fmt::Debug
impls have been adjusted to behave exactly like the delegating impls inparking_lot
.fmt::Display
impls have been added for all guard types which behave the same. This does change the format of debug impls, for which I'm not sure if we provide any guarantees.Closes #2442 since it duplicates some of those efforts. If we don't want map methods I can resurrect only the relevant changes.