-
Notifications
You must be signed in to change notification settings - Fork 12
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
Implement WeakMutex by using the sync::Weak #17
Conversation
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.
So this is just a non-owning version of Mutex
? Can you give an example of where you would find it useful?
Hi, pub struct Parent {
self_mutex: Option<WeakMutex<Parent>>,
}
pub struct Child {
// Weak
parent_mutex: WeakMutex<Parent>
}
impl Parent {
pub fn new() -> Mutex<Parent> {
let parent = Parent {self_mutex: None};
let mutex = Mutex::new(parent);
mutex.try_lock().unwrap().self_mutex = Some(Mutex::downgrade(&mutex));
mutex
}
pub fn create_child(&self) -> Mutex<Child> {
if let Some(m) = &self.self_mutex {
let child = Child {parent_mutex: m.clone()};
Mutex::new(child)
} else {
panic!("");
}
}
} So the parent need a WeakMutex to prevent the memory leak from the reference |
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 example! That makes sense. Could you please add some tests, doc comments, and a CHANGELOG entry?
Done. I change the name from |
src/mutex.rs
Outdated
@@ -140,12 +140,26 @@ struct Inner<T: ?Sized> { | |||
data: UnsafeCell<T>, | |||
} | |||
|
|||
/// `MutexWeak` is non-owning reference to a mutex like [`Weak`] for [`Arc`]. |
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.
"mutex" in this sentence should be a link. I suggest the following wording:
`MutexWeak` is a non-owning reference to a [`Mutex`]. `MutexWeak` is to `Mutex` as [`std::sync::Weak`] is to [`std::sync::Arc`].
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.
How I make a link to Mutex
. The problem is that doc link would depend on the specific like https://docs.rs/futures-locks/0.3.3/futures_locks/struct.Mutex.html.
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.
You can make a relative link like this:
[`Mutex`]: struct.Mutex.html
src/mutex.rs
Outdated
@@ -140,12 +140,26 @@ struct Inner<T: ?Sized> { | |||
data: UnsafeCell<T>, | |||
} | |||
|
|||
/// `MutexWeak` is non-owning reference to a mutex like [`Weak`] for [`Arc`]. | |||
/// | |||
/// # Example |
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.
s/Example/Examples/, for consistency.
@@ -226,8 +240,8 @@ impl<T> Mutex<T> { | |||
} | |||
|
|||
impl<T: ?Sized> Mutex<T> { | |||
pub fn downgrade(this: &Mutex<T>) -> WeakMutex<T> { | |||
WeakMutex {inner: sync::Arc::<Inner<T>>::downgrade(&this.inner)} | |||
pub fn downgrade(this: &Mutex<T>) -> MutexWeak<T> { |
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.
Don't forget to document this method, too.
} | ||
|
||
impl<T: ?Sized> MutexWeak<T> { | ||
pub fn upgrade(&self) -> Option<Mutex<T>> { |
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.
This method also needs a doc comment.
CHANGELOG.md
Outdated
@@ -1,3 +1,8 @@ | |||
## [Unreleased] - ReleaseDate | |||
### Added | |||
- Added `MutexWeak` by using the `sync::Weak` |
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 "by using ..." clause isn't easy to understand. It's also unnecessary. You can just say "Added MutexWeak
", or "Added MutexWeak
, a non-owning reference to a Mutex
.
Done. I hope that everything is fine now. :) |
src/mutex.rs
Outdated
#[derive(Debug)] | ||
pub struct MutexWeak<T: ?Sized> { | ||
inner: sync::Weak<Inner<T>>, | ||
} | ||
|
||
impl<T: ?Sized> MutexWeak<T> { | ||
/// Tries to upgrade the `MutexWeak` to `Mutex`. If the `Mutex` was dropped | ||
/// then the function return [`None`]. |
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.
Not necessary to provide a link to None
's definition.
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.
LGTM! Thanks for your contribution.
I saw that you are using an
Arc
in yourMutex
. So I implement theWeakMutex
like theWeak
struct in theArc
. To prevent that user wraps theMutex
again with anArc
to use theWeak
.