Added map associated function to MutexGuard that uses new MappedMutexGuard type #2472

sunjay · 2020-05-01T00:25:22Z

Part of #2471, extends the work of #2445. Both this PR and #2445 together close #2471.

This PR introduces the MappedMutexGuard type and adds a map associated function to MutexGuard. The work here is largely based on #2445, so thanks @udoprog for the great PR. This was very easy to implement using your work as a reference.

MappedMutexGuard works almost exactly the same as parking_lot::MappedMutexGuard, but adapted to the internals of tokio::sync::Mutex. The MappedMutexGuard type stores a reference to the semaphore from the original Mutex as well as a raw pointer *mut T that is the result of calling the function passed to map. The Mutex does not hold a mutable reference to its data (it uses an UnsafeCell), so I do not believe that it is possible to accidentally run into any aliasing issues.

I added documentation based on the work in #2445 and the documentation in parking_lot/lock_api. There are doctests in both map and try_map that are almost exactly the same as the ones in #2445 for RwLockWriteGuard. I generated the docs with cargo doc and everything looks great. 🎉

tokio/src/sync/mod.rs

Darksonn · 2020-05-11T20:21:06Z

What is the status on this?

sunjay · 2020-05-12T16:35:59Z

Thanks @carllerche! I have addressed @hawkw's comment and merged MappedMutexGuard into MutexGuard. Note that this required me to add a manual unsafe impl Send for MutexGuard. I believe that is correct (and #2445 does the same), but someone with a better understanding than me should also verify that.

This is ready for review again. 🎉

udoprog · 2020-05-12T17:25:37Z

Heyo. So in case we want to support a similar API as parking_lot, there is a distinction between the mapped and the regular guard.

For a quick comparison:

https://docs.rs/lock_api/latest/lock_api/struct.MutexGuard.html (has unlocked, unlocked_fair, and bump).
https://docs.rs/lock_api/latest/lock_api/struct.MappedMutexGuard.html

It would be nice to make sure that if we want to bring these methods into Tokio's lock API, we don't encounter similar issues, which is a consideration I've raised now in #2445 as well.

carllerche · 2020-05-12T19:00:59Z

@udoprog could you elaborate on why the APIs differ between the two?

hawkw

This looks correct to me, and not having a separate type for MappedMutexGuard looks nice. However, if @udoprog has additional context around the rationale behind having a separate type in parking_lot, I'd like to wait to hear about that before we merge this PR in its current state.

tokio/src/sync/mutex.rs

tokio/tests/sync_mutex.rs

tokio/src/sync/mutex.rs

hawkw · 2020-05-12T19:13:44Z

CI failures appear to be network timeouts while updating dependencies, not test failures. I restarted them.

udoprog · 2020-05-12T23:51:29Z

@udoprog could you elaborate on why the APIs differ between the two?

Yeah. I just haven't had time to do it yet. If someone else beats me to reviewing this part of parking_lot, feel free to beat me to it!

udoprog · 2020-05-13T13:43:58Z

Well. So far the issue seems to be that map provides the ability to project a subfield of MutexGuard<T> (as MutexGuard<U>), and for some reason &mut MutexGuard<U> doesn't seem to provide the necessary guarantee (on a type level?) that the original MutexGuard<T> is uniquely held by the current thread and can be soundly unlocked and mutated by another thread. But this is frankly over my head.

Maybe @Amanieu could provide some insight into what the potential soundness issue is if they have the time?

Amanieu · 2020-05-13T14:14:53Z

The main soundness issue is that a MappedMutexGuard can't temporarily give up the lock and the re-acquire it later while still pointing to the mapped value. This is because another thread can acquire the lock in the meantime and modify the locked data such that the map operation is now invalid. For example, if you mapped to a field of an enum, another thread could change that enum to a different variant which would make the original map invalid.

This can happen in several cases, which parking_lot supports only for MutexGuard and not MappedMutexGuard:

With Condvar.
With MutexGuard::unlocked.
With MutexGuard::bump.
For RwLocks, upgrading/downgrading a lock.

udoprog · 2020-05-13T14:24:43Z

@Amanieu That makes sense. Thank you!

With that in mind, I think it would be prudent to keep the separate guards so that we can mimic those APIs in the future without a major break.

@carllerche What do you say?

hawkw · 2020-05-13T17:21:59Z

We should also consider whether or not it's possible and makes sense for Tokio's async mutex to provide all the same methods as parking_lot's mutex.

carllerche · 2020-05-13T17:30:22Z

What are the use cases for mapping a mutex guard? I don't think I've ever used it.

sunjay · 2020-05-13T17:33:29Z

What are the use cases for mapping a mutex guard? I don't think I've ever used it.

The main use case is for getting a subfield of a value guarded by a mutex. See the documentation of map in the PR for some code that demonstrates that.

carllerche · 2020-05-13T17:53:12Z

@sunjay i understand this, but when would one want it guarded by the mutex vs. let v = &mut my_guard.field;.

Amanieu · 2020-05-13T17:54:32Z

Mapped guards are generally used when you are returning a guard as part of your public API, but you only want to expose a subset of your internal state to the user.

hawkw · 2020-05-13T17:59:40Z

Or, conversely, I imagine, when a function takes a MutexGuard<T> but you have a Mutex<StructThatHasATField>?

Darksonn · 2020-07-27T17:39:14Z

Is this waiting for #2445 to merge, or what is the status?

sunjay · 2020-07-27T17:52:07Z

This PR doesn't depend on any of the changes from #2445, so it can be merged as soon as a decision is made about whether we want a separate type for the mapped mutex guard. I'm guessing that we will choose not to have one based on #2445 (comment), but I defer that to the team members who know more about all of this than I do. :)

I'll rebase the PR shortly.

hawkw

Overall, this looks good to me, modulo a couple of very minor nits.

I still want to get @carllerche's final opinions on the overall direction of MappedXGuard changes.

tokio/src/sync/mutex.rs

sunjay

Apologies for the spam. I'm away from my computer so I'm fixing the build via GitHub suggestions from my phone.

tokio/src/sync/mutex.rs

Darksonn · 2021-01-12T09:58:53Z

I would like to remind people that this exists.

Darksonn · 2021-03-20T17:19:06Z

@sunjay Are you interested in getting this PR finished up?

sunjay · 2021-03-20T17:20:16Z

Sure, though I believe it is still waiting on an opinion from @carllerche

Darksonn · 2021-03-20T17:22:29Z

I will do a full review later today, and try to get Carl to look as well. (but that probably wont happen until monday I'm guessing)

Darksonn

This seems ok to me. We already have these methods for RwLock, and it follows the same design here.

Darksonn · 2021-03-20T18:53:36Z

There's a rustfmt failure, and you should probably merge in master to get the up-to-date CI config.

Darksonn · 2021-03-20T19:06:28Z

Looking more closely at this, I think we should have separate non-mapped and mapped guards, since if we don't, we can never implement an async Condvar due to the soundness issues that @Amanieu mentioned.

carllerche · 2021-03-20T22:36:28Z

I am deferring to @Darksonn and @hawkw

sunjay · 2021-03-25T03:04:37Z

Looking more closely at this, I think we should have separate non-mapped and mapped guards, since if we don't, we can never implement an async Condvar due to the soundness issues that @Amanieu mentioned.

The first commit on my PR has separate mapped guards. Should I go back to that version? Happy to do whatever you think is correct here.

Darksonn · 2021-03-25T08:04:23Z

Yeah, I think that having separate guard types is safer in the light of being able to add new features in the future.

Darksonn · 2021-04-12T11:24:20Z

Were you going to go back to the other version?

sunjay · 2021-04-12T15:12:28Z

Yes, sorry. Haven't had a chance to do so yet.

sunjay · 2021-04-19T03:26:26Z

@Darksonn sorry for the delay. I've gone back to the version with a separate MappedMutexGuard type. I've also rebased from the master branch and made some minor tweaks based on the feedback I got on that initial commit. This should be ready to review/merge now. Thanks for all your patience and for all the discussion! 😊

tokio/src/sync/mutex.rs

…warnings

…roduced `MappedMutexGuard` type

Darksonn · 2021-04-22T09:31:11Z

The history seems like you added back the commit that merged them, but the diff doesn't have it..? I'm very confused. Force pushes always make the history so confusing.

sunjay · 2021-04-22T14:27:13Z

Instead of reverting back to the first commit and losing the history, I just added a commit on top of everything else with the separate mapped muted guard type. So all the commits there right now are correct.

Darksonn

Thanks.

…exGuard MappedOwnedMutexGuard had an (as of yet unused) undefined behaviour case in its implementation of map_mut. This has been superceded by tokio's tokio-rs/tokio#2472 which used a better implementation without undefined behaviour (using UnsafeCell instead).

hawkw reviewed May 1, 2020

View reviewed changes

tokio/src/sync/mod.rs Show resolved Hide resolved

udoprog mentioned this pull request May 1, 2020

sync: Implement map methods of parking_lot fame #2445

Merged

Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-sync Module: tokio/sync labels May 1, 2020

sunjay force-pushed the mutexguard-map branch from 32173b2 to be65d27 Compare May 12, 2020 16:29

hawkw approved these changes May 12, 2020

View reviewed changes

tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved

tokio/tests/sync_mutex.rs Outdated Show resolved Hide resolved

tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved

sunjay force-pushed the mutexguard-map branch from dccdd69 to aaae6f1 Compare July 27, 2020 18:12

Darksonn requested a review from hawkw August 2, 2020 11:52

hawkw approved these changes Aug 3, 2020

View reviewed changes

tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved

tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved

sunjay commented Aug 3, 2020

View reviewed changes

tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved

tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved

tokio/src/sync/mutex.rs Show resolved Hide resolved

tokio/src/sync/mutex.rs Show resolved Hide resolved

Darksonn approved these changes Mar 20, 2021

View reviewed changes

sunjay force-pushed the mutexguard-map branch from ea543ae to caf052c Compare March 25, 2021 03:02

sunjay force-pushed the mutexguard-map branch from caf052c to 635adf7 Compare April 19, 2021 03:23

sunjay force-pushed the mutexguard-map branch from 635adf7 to 68a18e2 Compare April 19, 2021 05:00

Darksonn reviewed Apr 21, 2021

View reviewed changes

tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved

sunjay added 6 commits April 21, 2021 16:52

Added map method to MutexGuard that uses new MappedMutexGuard type

1437d1a

Merging MappedMutexGuard into MutexGuard

084eaf9

Making sure that lock guards don't get dropped right away in tests

babe719

Removed extra variables from tests that were added to avoid must_use …

a132956

…warnings

Re-implemented map associated function for MutexGuard with re-int…

54e64e0

…roduced `MappedMutexGuard` type

Updating Send and Sync impls on MappedMutexGuard to match parking_lot

161926c

sunjay force-pushed the mutexguard-map branch from 68a18e2 to 161926c Compare April 22, 2021 00:00

Darksonn approved these changes Apr 23, 2021

View reviewed changes

Darksonn merged commit 0ba1e3c into tokio-rs:master Apr 23, 2021

Darksonn mentioned this pull request May 14, 2021

Prepare Tokio v1.6.0 #3782

Merged

Darksonn mentioned this pull request Jan 31, 2023

OwnedMutexGuard is missing a mapped version #5414

Closed

Added map associated function to MutexGuard that uses new MappedMutexGuard type #2472

Added map associated function to MutexGuard that uses new MappedMutexGuard type #2472

Conversation

sunjay commented May 1, 2020

Darksonn commented May 11, 2020

sunjay commented May 12, 2020

udoprog commented May 12, 2020

carllerche commented May 12, 2020

hawkw left a comment

Choose a reason for hiding this comment

hawkw commented May 12, 2020

udoprog commented May 12, 2020

udoprog commented May 13, 2020 • edited Loading

Amanieu commented May 13, 2020

udoprog commented May 13, 2020

hawkw commented May 13, 2020

carllerche commented May 13, 2020

sunjay commented May 13, 2020

carllerche commented May 13, 2020

Amanieu commented May 13, 2020

hawkw commented May 13, 2020

Darksonn commented Jul 27, 2020

sunjay commented Jul 27, 2020

hawkw left a comment

Choose a reason for hiding this comment

sunjay left a comment

Choose a reason for hiding this comment

Darksonn commented Jan 12, 2021

Darksonn commented Mar 20, 2021

sunjay commented Mar 20, 2021

Darksonn commented Mar 20, 2021 • edited Loading

Darksonn left a comment

Choose a reason for hiding this comment

Darksonn commented Mar 20, 2021

Darksonn commented Mar 20, 2021

carllerche commented Mar 20, 2021

sunjay commented Mar 25, 2021

Darksonn commented Mar 25, 2021

Darksonn commented Apr 12, 2021

sunjay commented Apr 12, 2021

sunjay commented Apr 19, 2021

Darksonn commented Apr 22, 2021

sunjay commented Apr 22, 2021

Darksonn left a comment

Choose a reason for hiding this comment

udoprog commented May 13, 2020 •

edited

Loading

Darksonn commented Mar 20, 2021 •

edited

Loading