Clarify use of `unsafe` in extra::arc

[sfackler]

extra::arc::MutexArc contains two sets of methods to access the Arc's wrapped object. unsafe_access and unsafe_access_cond are tagged unsafe as "it is possible to construct a circular reference among multiple Arcs by mutating the underlying data. This creates potential for deadlock, but worse, this will guarantee a memory leak of all involved Arcs." There are also access and access_cond methods defined when the inner type is freezable as that guarantees the type can't contain a MutexArc. However, it's still trivially easy to deadlock using the safe methods:

extern mod extra;

use extra::arc::MutexArc;

fn main() {
    let arc1 = MutexArc::new(());
    let arc2 = arc1.clone();

    do arc1.access |_| {
        do arc2.access |_| {
        }
    }
}

It seems that the consensus in #2821 was that possibility of deadlock is inherently unsafe, so should access and access_cond be removed?

cc @bblum

[Thiez]

Perhaps access should fail when it is called by the same task twice?

It would be nice if people didn't have to use unsafe blocks just to get normal stuff done.

[sfackler]

Nested locks in the same task aren't the only way you can run into deadlock:

extern mod extra;

use std::cell::Cell;
use extra::arc::MutexArc;
use extra::comm::DuplexStream;

fn main() {
    let arc1 = MutexArc::new(());
    let arc2 = MutexArc::new(());

    let (stream1, stream2) = DuplexStream();

    let arc11 = Cell::new(arc1.clone());
    let arc21 = Cell::new(arc2.clone());
    let stream1 = Cell::new(stream1);
    do spawn {
        let arc1 = arc11.take();
        let arc2 = arc21.take();
        let stream = stream1.take();

        do arc1.access |_| {
            stream.send(());
            stream.recv();
            do arc2.access |_| {}
        }
    }

    let arc12 = Cell::new(arc1);
    let arc22 = Cell::new(arc2);
    let stream2 = Cell::new(stream2);
    do spawn {
        let arc1 = arc12.take();
        let arc2 = arc22.take();
        let stream = stream2.take();

        do arc2.access |_| {
            stream.send(());
            stream.recv();
            do arc1.access |_| {}
        }
    }
}

[Thiez]

I am aware of that, but it seems to me this particular error would be easy and cheap to catch, so if we aren't doing that yet, we probably should.
Personally I don't think the possibility of a deadlock should require an unsafe block.

[bblum]

Consensus in the past has been that deadlock is not unsafe (#2821). You can deadlock with pipes too, with just two lines of code.

The non-Freeze accessors for MutexArc are unsafe for a totally different reason, which is documented in https://github.com/mozilla/rust/blob/master/src/libextra/arc.rs#L201 .

Regarding "cheap and easy to catch" - I don't believe we should try to fail in the single-task deadlock case unless we can reliably fail in all deadlock cases (which is not out of the question but requires scheduler support, see #7889). A user who is used to seeing a failure message from their previous attempts which had deadlocked would be all the more confused when they find their cross-task deadlocking code does not fail.

[sfackler]

Is the possibility of memory leakage really considered unsafe? RcMut doesn't seem to share that assumption, but I may be missing something there.

[thestinger]

Neither Rc or RcMut provides a way to leak memory via the safe constructors.

[thestinger]

We currently don't officially consider leaks to be unsafe, but I think leaking past the lifetime of tasks with references is a very questionable thing to allow.

[bblum]

The box annihilator is not a good solution to rc reference cycles, but it is at least a solution. Cycles of MutexArcs will produce valgrind errors.

[emberian]

Rc now provides a way to leak with the safe constructors.