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

Implement WeakMutex by using the sync::Weak #17

Merged
merged 9 commits into from
Apr 21, 2019
Merged

Implement WeakMutex by using the sync::Weak #17

merged 9 commits into from
Apr 21, 2019

Conversation

LinkTed
Copy link
Contributor

@LinkTed LinkTed commented Apr 21, 2019

I saw that you are using an Arc in your Mutex. So I implement the WeakMutex like the Weak struct in the Arc. To prevent that user wraps the Mutex again with an Arc to use the Weak.

Copy link
Owner

@asomers asomers left a 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?

@LinkTed
Copy link
Contributor Author

LinkTed commented Apr 21, 2019

Hi,
the following problem I have right now:
When you have a parent class, which has a mutex from himself, because if the parent wants to create a child object and the child object need a mutex to the parent then you have a reference cycle problem. Example code:

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 self_mutex.

Copy link
Owner

@asomers asomers left a 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?

@LinkTed
Copy link
Contributor Author

LinkTed commented Apr 21, 2019

Done. I change the name from WeakMutex to MutexWeak to keep the naming convention.

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`].
Copy link
Owner

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`].

Copy link
Contributor Author

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.

Copy link
Owner

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
Copy link
Owner

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> {
Copy link
Owner

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>> {
Copy link
Owner

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`
Copy link
Owner

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.

@LinkTed
Copy link
Contributor Author

LinkTed commented Apr 21, 2019

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`].
Copy link
Owner

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.

Copy link
Owner

@asomers asomers left a 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.

@asomers asomers merged commit 644b4d2 into asomers:master Apr 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants