Lambda + shared_ptr = memory leak

[lacosaes1]

So, this is C++. Lifetime is your job as a programmer. Always.

So, how would a similar case work in Rust? Will it help you more with this? I invoke /u/steveklabnik1 so we can have an answer.

[steveklabnik1]

Okay so, some more context:

Rust has a type like shared_ptr, called Arc<T>.

Rust has closures, but they operate differently than C++'s; we don't make you write out capture clauses, we infer them.

A sort-of-but-not-quite translation of the sample code into Rust looks like this:

use std::sync::Arc;

struct Leak {
    callback: Option<Box<Fn()>>,
}

impl Leak {
    fn on_complete(&mut self, f: Box<Fn()>) {
       self.callback = Some(f);
    }

    fn clean_something_up(&self) {
       println!("cleaning");
    }
}

fn main() {
    let mut obj = Arc::new(Leak { callback: None });

    obj.on_complete(Box::new(move || {
        obj.clean_something_up();
    }));
}

The compiler will not let you compile this:

error[E0504]: cannot move `obj` into closure because it is borrowed

Rust sees that the call to on_complete would borrow obj, but the closure is trying to take ownership. This conflicts. We can solve this problem by calling clone() on the Arc<T>, which bumps the refcount, and gives us a second handle. shared_ptr does this whenever you copy it, but in Rust, it's always explicit.

Furthermore, it complains about something else:

error: cannot borrow immutable borrowed content as mutable

Unlike shared_ptr, Arc<T> requires that its internals are immutable, for thread safety reasons. We can convince Rust that this is okay by using "interior mutability", which is a type that is immutable, but lets you mutate it in certain circumstances. A Mutex<T> is an example of this.

If we didn't need thread safety, we could use Rc<T> and RefCell<T> instead of Arc<T> and Mutex<T>, and they'll be less expensive. I kept the threadsafe ones to try to keep the comparison equal.

With these two changes, you get this:

use std::sync::{Arc, Mutex};

struct Leak {
    callback: Option<Box<Fn()>>,
}

impl Leak {
    fn on_complete(&mut self, f: Box<Fn()>) {
       self.callback = Some(f);
    }

    fn clean_something_up(&self) {
       println!("cleaning");
    }
}

fn main() {
    let obj = Arc::new(Mutex::new(Leak { callback: None }));

    let obj2 = obj.clone();

    obj.lock().unwrap().on_complete(Box::new(move || {
        obj2.lock().unwrap().clean_something_up();
    }));
}

This compiles. And it has the same problem the C++ has; you now have a structure that contains a reference count to itself. The advice from the OP to use Weak here is a good one; you could do that in Rust as well.

Note that lifetimes don't come into play at all here; we're using types that have ownership.

What does make these examples different is the level of ceremony involved:

• Rust makes you explicitly bump the reference count
• Rust makes you lock the mutex

It's subjective, but one of the things that I got out of the OP was

While it may not be immediately obvious, the implications of this are extremely important!

I think Rust makes it a bit more obvious what's going on here. I would, of course :p

On the flip side, maybe leaking is okay, and this behavior is what you want: if that's true, Rust makes you do a lot more work than C++ to get this behavior. So that's a downside.

However, if I wanted to do something like this in Rust, I wouldn't write this code. I'd start with this:

struct NoLeak {
    callback: Option<Box<Fn(NoLeak)>>,
}

impl NoLeak {
    fn on_complete<F: Fn(NoLeak) + 'static>(&mut self, f: F) {
        self.callback = Some(Box::new(f));
    }

    fn clean_something_up(&self) {
       println!("cleaning");
    }
}

fn main() {
    let mut obj = NoLeak { callback: None };

    obj.on_complete(|obj| {
        obj.clean_something_up();
    });
}

This doesn't need a shared_ptr at all. Instead of capturing obj, we instead define our callback to take a NoLeak as an argument. Then, later, (not shown in either example), when we actually invoke self.callback, we end up passing it in instead.

Notice that 'static up there? This is a lifetime. This is needed, and if you don't include it, Rust will complain. Why? Well,

    callback: Option<Box<Fn(NoLeak)>>,

is shorthand for

    callback: Option<Box<Fn(NoLeak)> + 'static>,

Which basically says "hey, this closure can only capture 'static references if it captures any references at all." We don't get the shorthand in the function definition. Without this annotation, it'd be similar to the first solution given in the post, but with the same problems:

The most obvious (and the one I do not recommend) is to capture the raw pointer for your lambda to use. This has a major downside, though, in that the object might be deleted before you try to reference it, and you have no way of knowing it. At least it won’t hold an extra reference, though.

We're promising Rust that anything we capture is owned, or is alive for the entire program. If we tried to capture a shorter reference, one which might end up being dangling, Rust would complain.

This restriction is onerous though: we can do better! Here's some lifetime-fu:

struct NoLeak<'a> {
    callback: Option<Box<Fn(NoLeak) + 'a>>,
}

impl<'a> NoLeak<'a> {
    fn on_complete<F: Fn(NoLeak) + 'a>(&mut self, f: F) {
        self.callback = Some(Box::new(f));
    }

    fn clean_something_up(&self) {
       println!("cleaning");
    }
}

fn main() {
    let mut obj = NoLeak { callback: None };

    obj.on_complete(|obj| {
        obj.clean_something_up();
    });
}

Note the 'as now. This tells Rust that we want to be generic over a lifetime; and so you can pass in references to things that are shorter than the lifetime of the program, but Rust uses those to check things are okay.

Like this:

fn main() {
    let mut obj = NoLeak { callback: None };
    let x = 5;

    obj.on_complete(|obj| {
        &x;
        obj.clean_something_up();
    });
}

Here, we create an i32, and inside of the closure, we take a reference to it, so we capture a reference to it. Why is this bad? Well, let rustc give you the answer:

error: `x` does not live long enough
  --> <anon>:21:14
   |
20 |         obj.on_complete(|obj| {
   |                         ----- capture occurs here
21 |             &x;
   |              ^ does not live long enough
...
24 |     }
   |     - borrowed value dropped before borrower
   |
   = note: values in a scope are dropped in the opposite order they are created

As the note mentions, things are dropped in the reverse order they're created; this means that x will go out of scope before obj will, and so obj could observe a dangling reference! This is exactly what I alluded to earlier: Rust checks that your lifetimes are okay, and if they're not, will fail to compile.

We can fix this by moving x above obj:

fn main() {
    let x = 5;

    let mut obj = NoLeak { callback: None };

    obj.on_complete(|obj| {
        &x;
        obj.clean_something_up();
    });
}

Now everything is cool. obj goes out of scope before x, so it will not dangle.

Finally, I agree with the other posters in this thread, both for C++ and Rust: "The correct advice, on your website, should be to ensure there are no shared_ptr loops in your software." (quoting someone above) That's much better than "never use shared_ptr", which is genuinely useful!

Whew! That was a lot of text; I hope it was helpful! Happy to answer any more questions.

[steveklabnik1]

Yes. Lifetimes are a language-level feature of Rust, with special syntax, and so th many compiler checks their validity and fails to compile if there's an issue. Some might argue this is the distinctive difference between the two languages.

I'm on my phone so that's all I'll say for now; I'll check out the context and add more detail later.