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

Mutex guard lock ref recovery methods #3928

Merged
merged 2 commits into from Jul 20, 2021

Conversation

ijackson
Copy link
Contributor

@ijackson ijackson commented Jul 5, 2021

Motivation

There are various situations where it is useful to be able to recover &Mutex from a MutexGuard. One of them is condition variables (see #3892 and my new async-condvar-fair crate) but there are other possibilities.

Solution

Broadly, for the applicable guard types, and where possible:

impl Guard<'l, T> {
    pub fn mutex(this: &Self) -> &'l Mutex<T> { self.lock }
}

(Mutatis mutandis for the various mutex types - varies eg for Owned and RwLock.)

In #3741 a method was proposed to convert the guard into the mutex reference. Obtaining a borrow is strictly more general, and is what is implemented in parking_lot and smol for their mutex types.

Open questions

  • Are these the best names? parking_lot uses fn mutex() for its mutex type but does not provide these facilities for its rwlocks. smol uses lock() source() which is more general, leading to less variation between Mutex and RwLock.

  • Was I right to use #[tokio::main] async fn main(){...} for my doctests? The contribution guide suggests something involving block_on but the other tests in mutex.rs used #[tokio::main] so I chose that.

  • I wasn't sure whether to # out the uses at the top of the doctests. I find them noisy here - in this particular case the intended types are extremely obvious.

  • I have skipped RwLockReadGuard and RwLockWriteGuard. This is not because I think they wouldn't be useful, but those structs contain references to pieces of the rwlock, not the whole rwlock, and I wasn't sure how to recover a suitable reference (or whether that's even possible). The (straightforward portions of) the corresponding feature for RwLock is now RwLock guard lock ref recovery methods, owned guards #3930.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Jul 6, 2021
@Darksonn
Copy link
Contributor

Darksonn commented Jul 6, 2021

I think having a method like this makes sense. Some comments:

  1. I prefer to add the method to all RwLock guards, or to none. It should be possible to add it to the remaining guards by changing the &'a Semaphore field to &'a RwLock.
  2. We should not call the method lock(). That is confusing since we already have a Mutex::lock method, and that method does something different.
  3. Using #[tokio::main] async fn main(){...} for the doctests is fine, though all the other doctests put the example directly in main rather than define a new method. I think I'm ok either way, but putting it directly in main reduces the amount of hidden code.
  4. I'd like to include the imports in the doctests. Without them, the example would not compile if pasted into the playground.
  5. Please run the doctests through rustfmt for more consistent formatting with the rest of the tests.
  6. Please end the sentences in the documentation with a period.

@ijackson
Copy link
Contributor Author

ijackson commented Jul 6, 2021

I think having a method like this makes sense. Some comments:

  1. I prefer to add the method to all RwLock guards, or to none. It should be possible to add it to the remaining guards by changing the &'a Semaphore field to &'a RwLock.

I tried this. But the guard returned from RwLock*Guard::map needs to be RwLockReadGuard<U> but it needs to contain &RwLock<T>. So it would have to have an additional generic parameter like OwnedRwLock*Guard. Presumably this is why these types currently contain &Semaphore. The other libraries that provide functions for recovering the mutex type have separate mapped guard types.

Is it OK to add that new generic (with the default, like for OwnedRwLock*Guard)? I think that would be a breaking change, because currently a program might use RwLock*Guards with different Ts ,but the same U, interchangeably. Currently they're the same type, but they would become different types.

  1. We should not call the method lock(). That is confusing since we already have a Mutex::lock method, and that method does something different.

Good point.

I will address your other comments. I'm inclined to split the RwLock versions into a separate MR to unblock the ones on Mutex.

@ijackson
Copy link
Contributor Author

ijackson commented Jul 6, 2021

  1. We should not call the method lock(). That is confusing since we already have a Mutex::lock method, and that method does something different.

Good point.

Oh: another possible colour for this bikeshed. smol calls this function source.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
@ijackson ijackson changed the title Lock guard lock ref recovery methods Mutex guard lock ref recovery methods Jul 6, 2021
@ijackson
Copy link
Contributor Author

ijackson commented Jul 6, 2021

  1. I prefer to add the method to all RwLock guards, or to none. It should be possible to add it to the remaining guards by changing the &'a Semaphore field to &'a RwLock.

I split this off - see #3930. Thanks.

  1. We should not call the method lock(). That is confusing since we already have a Mutex::lock method, and that method does something different.

I was wrong when I said that smol uses lock() for this; it uses source(). Let me know what name you prefer and I will paint the bikeshed accordingly.

  1. Using #[tokio::main] async fn main(){...} for the doctests is fine, though all the other doctests put the example directly in main rather than define a new method. I think I'm ok either way, but putting it directly in main reduces the amount of hidden code.

Thanks. I prefer to have a self-contained function as it avoids the example being cluttered with setup code, so I have left this as-is.

  1. I'd like to include the imports in the doctests. Without them, the example would not compile if pasted into the playground.
  2. Please run the doctests through rustfmt for more consistent formatting with the rest of the tests.
  3. Please end the sentences in the documentation with a period.

Done.

Is there some automatic way to run rustfmt on doctests? I resorted to cut-and-paste plus this rune: perl -pe 's{^ */// ?(\# ?)?}{}' <t.rs >u.rs && cp u.rs v.rs && rustfmt --edition=2018 v.rs && diff -u u.rs v.rs

@Darksonn
Copy link
Contributor

Darksonn commented Jul 6, 2021

Eh, I prefer fn mutex() over fn source().

@ijackson
Copy link
Contributor Author

ijackson commented Jul 6, 2021

Eh, I prefer fn mutex() over fn source().

Cool, no repainting needed then. Anything else you'd like me to do to this MR?

@Darksonn
Copy link
Contributor

Darksonn commented Jul 6, 2021

I think it seems ok. I'll wait for the americans to get a chance to review it though.

@Darksonn Darksonn merged commit 2520048 into tokio-rs:master Jul 20, 2021
@Darksonn Darksonn mentioned this pull request Jul 22, 2021
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-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants