SharedChan can be used to create cycles

[bill-myers]

Here is how one does it:

1. Create channels A and B
2. Send a clone of A's SharedChan and send A's port to channel B
3. Send a clone of B's SharedChan and send B's port to channel A
4. Drop the original SharedChans
5. Memory leak!

The Linux kernel has the exact same issue with file descriptor passing on Unix sockets and they solve it with an global in-kernel garbage collector for unix sockets (http://lxr.free-electrons.com/source/net/unix/garbage.c)

Rust can't do the GC approach easily because it can pass structures containing SharedChans in addition to just SharedChan like Linux.

However, we can prevent cycles just like we do for RWARC: by adding a Freeze bound to SharedChan's T and making sure SharedChan is no_freeze.

Should this be done?

If not, why shouldn't the Freeze be removed from RWARC too, since they are effectively equivalent functionality? (method calls on RWARC-protected objects are equivalent to sending a message via SharedChan to a task that calls the corresponding method)

[pcwalton]

I don't think it's worth it. I would like to remove that bound on RcMut.

[bill-myers]

@pcwalton That seems extremely dangerous: won't it risk newbie programmers familiar with GC languages (i.e. all popular languages except C/C++/ObjC) using RcMut/RWARC as if it were a GC pointer and creating programs that leak gobs of memory?

SharedChan is bit harder to misuse this way though, since while object graphs referencing each other with RWARC and calling methods on each other are equivalent to task graphs referencing each other with SharedChan and sending synchronous messages to each other, task graphs are more expensive and harder to implement.

[alexcrichton]

I feel like I'm still a little fuzzy on how this can acutally become a problem, can you post some code which exhibits the leak?

[thestinger]

@pcwalton: AFAICT, without the bound of Freeze or Send, RcMut can be used to cause dangerous cycles with managed pointers where destructors are run too many times.

[bill-myers]

@alexcrichton Something like this (not tested whether it compiles):

struct Foo
{
c: SharedChan<Foo>,
p: Port<Foo>
}

fn leak_memory()
{
let (ap, ac) = SharedChan::new();
let (bp, bc) = SharedChan::new();
let a = Foo {p: ap, c: ac.clone()};
let b = Foo {p: bp, c: bc.clone()};
ac.send(b);
bc.send(a);
}

[bill-myers]

Or in fact, even simpler:

struct Foo
{
c: SharedChan<Foo>,
p: Port<Foo>
}

fn leak_memory()
{
let (p, c) = SharedChan::new();
let x = Foo {p: p, c: c.clone()};
c.send(x);
}

[thestinger]

A solution to this: #10837

[pcwalton]

@bill-myers I don't see it as extremely dangerous. It's just something you have to learn. In Servo I found the Freeze bound on MutexArc to be very annoying because it tries too hard to prevent cycles, despite the fact that I know what I'm doing, and I just wrote a safe wrapper around unsafe_access because otherwise I just couldn't do what I needed to do.

We have never prevented memory leaks in Rust. You can always leak memory by, for example, having two tasks in an infinite loop ping ponging each other repeatedly. We just make it hard to leak memory in practice because we encourage use of data structures that don't leak (unique ownership, and in the future Gc pointers). Trying too hard to prevent memory leaks just leads to people who know what they're doing write annoying unsafe wrappers.

[pcwalton]

@thestinger Seems to me we should solve that problem then, perhaps by moving to Hans Boehm's suggestion of how to handle destructors in managed pointers (don't run any destructors on managed pointers until all objects are judged dead). The bound of Freeze on RcMut is clearly untenable since we want to remove RcMut.

[thestinger]

@pcwalton: A cycle checking lint would have an easy opt-out unlike the Freeze or Send bounds. It would be a lot more precise by actually checking that the type tagged with #[cycle_check] recursively references itself.

[sfackler]

@pcwalton RcMut is already gone. Rc has constructors to build from Freeze, Send and RefCell<T> where T is Freeze as well as an unsafe constructor with no bounds.

[pcwalton]

@thestinger We tried that in very early versions of Rust. You end up having to be so conservative around fn and &Trait (basically: existential types) that it's not worth it.

[thestinger]

@pcwalton: Ah, that makes sense. It's a bit sad that we won't be offering anything better than C or C++ in regards to this though.

[thestinger]

I'm fully convinced that it's better to leave off the Freeze bound because it's crippling for mutable concurrent data structures. By definition, a mutable data structure without one owner is non-Freeze. I think the need to store a concurrent queue (channel), vector, map or a MutexArc in another concurrent data structure will be quite common. Moving all of the container references you're going to need into the task on spawn is asking too much.

It's sad that we can't do something better here, but doing nothing is better than crippling these types.

[bill-myers]

I'm convinced that attempting to prevent this statically is indeed wrong.

However, I think there needs to be a leak detector that detects if any SharedChans and similar are still alive at program termination and prints an error.

This should result in catching most leaks in testing, although it won't help programs that cause leaks either with very low probability or on paths there are not covered by testing.

The user can then use valgrind --track-origins to figure out where the leaked allocation happened.

[alexcrichton]

Closing, we have decided to remove the Freeze or Send bounds from Rc, officially moving in a direction of "rust isn't trying to statically prevent cycles"