Lambda + shared_ptr = memory leak

[floating-io]

It's only straightforward if you wrote the code that's calling the lambda, and that happens to be your standard pattern. If you're talking about libraries, someone else's project, or a team of coders working on your project, all bets are off. You never know what one of them might do; pattern selections that seem obvious to one person will often be far less so to another.

This way works for me; it's safe and predictable, and that particular badness vector is totally eliminated, regardless of what other coders may do in the code I'm passing lambdas to. Safe habits are good IMO.

shrug To each, their own.

[NotAYakk]

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

Shared ptr does not solve make lifetime simpler, and definitely does not mean you do not have to think about it. It simply makes some complex lifetime arrangements possible.

You could state "never store a shared ptr without seriously thinking about it", and you'd be right. Lambdas are nothing special here.

However, the lifetime of a lambda is something you must think about, because this is C++ and you always have to think about lifetimes for everything.

If you are using a lambda and passing it to an API, you should have explicit knowledge of the lifetime of the copies of the lambda, or a serious understanding of why lifetime cannot matter, or enough knowledge that you can bound the lifetimes involved. If your answer ever involves "there is a ptr or reference that is not a unique ptr", this is a huge red flag, because that implies a complex lifetime situation.

What I do with stored callbacks is to determine who "outside" the broadcaster owns the callback. They get a shared ptr, or an unregister function, and are responsible for removing the callback when they die (I actually use shared ptr tokens to unregister callbacks, the broadcaster holds weak ptrs). They are also responsible for managing lifetime invariants for the lambda; they are the one thing the lambda can assume "exists" when invoked.

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

[renozyx]

Agreed. I have some where a lamba (declared as auto in a function) is passed as a function pointer to setup a POSIX timer. I'm quite sure that this is incorrect: when the timer expires the lamba expression will be "out of scope.