Guaranteed non-static destructors

[rkjnsn]

Guarantee that the destructor for data that contains a borrow is run before any
code after the borrow’s lifetime is executed.

Rendered

[rkjnsn]

Edit: This attempt is unsound because an Rc can outlive it's guard. See this post for a more correct ScopedRc implementation.

Here's an initial attempt at implementing an RcGuard for Rc: https://gist.github.com/rkjnsn/b5bfe2ca8a1758ee6b2a

Use it like this:

use std::rc::{Rc, RcGuard};
use std::cell::RefCell;

struct Cycle<'a> {
    next: RefCell<Option<ScopedRc<'a, Cycle<'a>>>>,
    val: &'a char,
}

impl<'a> Drop for Cycle<'a> {
    fn drop(&mut self) {
        println!("{}", self.val);
    }
}

fn main() {
    let a_val = 'a';
    let b_val = 'b';
    let c_val = 'c';
    let guard = RcGuard::new();
    let a = guard.new_rc(Cycle { next: RefCell::new(None), val: &a_val });
    let b = guard.new_rc(Cycle { next: RefCell::new(None), val: &b_val });
    let c = guard.new_rc(Cycle { next: RefCell::new(None), val: &c_val });
    *a.next.borrow_mut() = Some(b.clone());
    *b.next.borrow_mut() = Some(c.clone());
    *c.next.borrow_mut() = Some(a.clone());

    // Cycle is cleaned up when guard is dropped
}

[rkjnsn]

Heres an experiment with a NoCycleRc. There are almost certainly better ways to accomplish this... https://gist.github.com/rkjnsn/642766e1d81925e2540b

use nocyclerc::{NoCycleRc, NoCycleMarker};

struct Cycle<'a, T> {
    next: Option<NoCycleRc<'a, T>>,
    val: &'a char,
}

impl<'a, T> Drop for Cycle<'a, T> {
    fn drop(&mut self) {
        println!("{}", self.val);
    }
}

fn main() {
    // Rc's created with marker1 must only contain borrows that live longer than
    // marker1 and must not themselves outlive marker1.
    // NoCycleMarker is a zero-size type, and thus may be passed and cloned for
    // free
    let marker1 = NoCycleMarker::new();
    let a_val = 'a';
    let b_val = 'b';
    let c_val = 'c';

    // Error: a_val does not outlive marker1
    //let a = marker1.new_rc(Cycle { next: RefCell::new(None), val: &a_val });

    let marker1b = marker1.clone(); // Clones have the same lifetime

    // Error: a_val does not outlive marker1b (same lifetime as marker1)
    //let a = marker1b.new_rc(Cycle { next: RefCell::new(None), val: &a_val });

    let marker2 = NoCycleMarker::new(); // New, shorter lifetime

    // Ok: a_val and b_val outlive marker2
    let mut a = marker2.new_rc(Cycle::<Cycle<()>> { next: None, val: &a_val });
    let mut b = marker2.new_rc(Cycle::<()> { next: None, val: &b_val });

    // Error: b doesn't outlive marker2
    //nocyclerc::get_mut(&mut a).unwrap().next = Some(b.clone());

    let marker3 = NoCycleMarker::new();

    let mut c = marker3.new_rc(Cycle { next: None, val: &c_val });

    // Ok: b outlives marker3
    nocyclerc::get_mut(&mut c).unwrap().next = Some(b.clone());
}

[sfackler]

Panicing in Rc's Deref is a bit unfortunate since it'll end up being an "invisible" source of process aborts in destructors for things in Rc cycles. Might not be the end of the world since I can't imagine that non-'static Rc cycles will be all that common.

Given that that panic can only be triggered in a destructor (I think?) which will end up being a process abort, it might be worth it to explicitly abort there to avoid panic bloat at deref callsites.

[rkjnsn]

@sfackler It's also possible to pass an Rc up the stack to live longer than the guard, and then try to dereference it. This is safe, since it panics, but still rather unfortunate. I couldn't figure out any way to prevent an Rc from outliving its RcGuard (even with a custom Rc type) and still allow cycles. (In fact, the NoCycleRc exploits this fact.)

[reem]

It would really help to have an implementation, or at least the signatures and types of all the parts of this API. It's quite difficult to judge the ergonomics and such of this proposal without them.

[reem]

Read the comments: that helps, some of that stuff should move into the RFC.

[pythonesque]

How could this RFC be adapted to Arc? Is there a safe time to perform cycle collection? If not, does this then imply that channels would not be able to be used with scoped threads (since they use Arc)?

[reem]

Conceptually, this feels like making Rc just nullable arena pointers, since dereferencing them can panic if the RcGuard has been dropped (which is a dynamic, not static, condition). I'm not really sure yet how I feel about this, I have to mull it over some more.

[pythonesque]

dereferencing them can panic if the RcGuard has been dropped (which is a dynamic, not static, condition).

Doesn't this describe weak pointers? It's not clear to me whether under this proposal Rc actually has any benefit over TypedArena, to be honest, since the primary advantage of Rc there is drop latency, and the purpose of weak pointers is to break cycles at runtime (rather than use them from the start).

[rkjnsn]

It's also possible to pass an Rc up the stack to live longer than the guard, and then try to dereference it. This is safe, since it panics, but still rather unfortunate. I couldn't figure out any way to prevent an Rc from outliving its RcGuard (even with a custom Rc type) and still allow cycles.

After further thought, allowing Rc's to outlive the guard is, in fact, unsound (you could deref and then drop the guard). However, I think I have now found a way to allow cycles while keeping the Rc objects from outliving their guard. Let me update my code to test.

[lilyball]

The whole idea of RcGuard is fundamentally flawed. A reference-counting scheme cannot run destructors on cycles. It's wildly unsafe. This should be trivially obvious when you realize that a cycle, by definition, means both values have references to each other, and so you cannot possibly decide which value to destruct first because the second value would then be holding onto a reference to an already-destroyed value. The only possible way to proceed in this case is to detect when the second value attempts to access the first and immediately panic. And that seems like a really terrible solution.

Not only that, but who owns the RcGuard? Typically when you have an Rc cycle it's because there's no clear chain of ownership, and so both values must "own" each other (and then something else hopefully owns one, or both, of those values). So given that, there's no clear owner for the RcGuard, because if there were then you'd have a clear chain of ownership and wouldn't need the cycle.

research the possibility of a reference-counted type that statically disallows cycles

I believe that's known to be impossible, at least not without unacceptably limiting what values can be held in the Rc.

[rkjnsn]

@kballard

The only possible way to proceed in this case is to detect when the second value attempts to access the first and immediately panic. And that seems like a really terrible solution.

That's the plan, though I don't understand why it's terrible. It seems no different than RefCell::borrow_mut panicking when the cell is already borrowed. It can only ever happen if the user creates a cycle, doesn't clean it up themselves, and then tries to access the next link in the cycle in the destructor. It seems reasonable to call this a programming error and panic.

Not only that, but who owns the RcGuard?

If the contents are 'static, then cycles are fine and you don't need a guard. If the content's are not 'static, then they reference something anchored somewhere, and you can put the guard there. I don't think this is a problem.

I believe that's known to be impossible, at least not without unacceptably limiting what values can be held in the Rc.

In the general case, yes, but it can be useful for some cases, which is why I'm also suggesting a ScopedRc. I see it as similar to RefCell versus the borrow checker: check statically when you can, dynamically when you can't.

[rkjnsn]

@pythonesque

How could this RFC be adapted to Arc? Is there a safe time to perform cycle collection? If not, does this then imply that channels would not be able to be used with scoped threads (since they use Arc)?

The idea is that all non-cycle Arcs will be guaranteed by the borrow checker to have been dropped before the guard's destructor runs, so the guard will have exclusive access and collection will be safe.

It's not clear to me whether under this proposal Rc actually has any benefit over TypedArena, to be honest, since the primary advantage of Rc there is drop latency, and the purpose of weak pointers is to break cycles at runtime (rather than use them from the start).

The programmer is free to use weak pointers or avoid cycles all together to get all the benefits of reference counting. I don't expect a graph containing a bunch of cycles that stick around the entire time to be common (though it could be done). The advantage of having a guard is to ensure that if the programmer does create cycles of non-'static objects (which many comments on #1066 argue is a mandatory capability), the guarantee laid out by this RFC is still upheld.

[eddyb]

Specifying "potential for data revalidation on destruction" semantics for "objects with destructors and which are borrowing said data" is something I've previously discussed with @nikomatsakis.
In the cases I stumbled onto (thanks to a newbie who was questioning the strictness of borrowing rules), the presence of such a resource in a scope would imply that immediately exiting (e.g. return) the scope has the chance to revalidate the borrowed data, and thus immutable access to the underlying data might be unsafe.
But if there is no such resource, the data must be valid (because exiting the scope with borrows on that data would reallow access to it without any changes), and as such, one could temporarily borrow the data immutably while blocking mutable access to other transitive borrows.
Just so there is some context - this should be safe because the iterators don't have destructors to revalidate the vector, so the vector must be valid at any point:

fn attract(a: &Entity, b: &Entity) -> Vec3 {...}
let mut vec: Vec<Entity> = ...;
for a in &mut vec {
    let mut accel = Vec3(0.0, 0.0, 0.0);
    for b in &vec { // inside this loop, *a is immutable
        aceel += attract(a, b);
    }
    a.velocity += accel * dt;
}

Another thing to note is that data races wouldn't be possible, because thread::scoped produces a resource that borrows the data (and "revalidates" it via synchronization).

But not if you can safely forget that resource. Guaranteeing non-static destructors (or the revalidation subset, at least) always run is a must if you want to extend the borrow checker to understand more safe patterns, IMHO.

Which... reminds me, don't we have a cycle detector in the compiler? Can't we mix it with checking for interior mutability and non-static references and expose it as a trait? Or has that been proposed already?

[rkjnsn]

Okay, here we go: this gist correctly enforces lifetimes and I believe it is sound.

Sample usage:

use scopedrc::{ScopedRc, ScopedRcGuard};
use std::cell::{Cell, RefCell};

struct Cycle<'a> {
    next: RefCell<Option<ScopedRc<'a, Cycle<'a>>>>,
    val: &'a char,
}

impl<'a> Drop for Cycle<'a> {
    fn drop(&mut self) {
        println!("{}", self.val);
    }
}

fn main() {
    //let guard1 = ScopedRcGuard::new();

    let a_val = 'a';
    let b_val = 'b';
    let c_val = 'c';

    // Error: a_val does not outlive marker1
    //let a = guard1.new_rc(Cycle { next: RefCell::new(None), val: &a_val });

    // Error: guard2 does not outlive target
    //let target

    let guard2 = ScopedRcGuard::new();

    // Ok
    let target;

    let a = guard2.new_rc(Cycle { next: RefCell::new(None), val: &a_val });
    let b = guard2.new_rc(Cycle { next: RefCell::new(None), val: &b_val });
    let c = guard2.new_rc(Cycle { next: RefCell::new(None), val: &c_val });
    *a.next.borrow_mut() = Some(c.clone()); // Cycle
    *b.next.borrow_mut() = Some(a.clone()); // Holds cycle, but isn't one
    *c.next.borrow_mut() = Some(a.clone()); // Cycle

    // Error: guard2 is borrowed
    //drop(guard2);

    target = b;

    // At end of scope, &'b' is dropped immediately. The rest are cleaned up by
    // the cycle collector.
}

[pythonesque]

The programmer is free to use weak pointers or avoid cycles all together to get all the benefits of reference counting. I don't expect a graph containing a bunch of cycles that stick around the entire time to be common (though it could be done). The advantage of having a guard is to ensure that if the programmer does create cycles of non-'static objects (which many comments on #1066 argue is a mandatory capability), the guarantee laid out by this RFC is still upheld.

Weak pointers and avoiding cycles do not combine to give you all the benefits of reference counting. The whole point of reference counting cycles is to be able to break them dynamically--you want all the references to be strong most of the time. You can replicate the functionality in this RFC in Rust already with a TypedArena; you shouldn't be using Rc if you know when things are going to be dropped.

This RFC also adds potentially failing operations on Deref, which is invisible (Weak pointers deliberately don't do that!). Your version of Rc is also quite a bit larger than the existing one and RcGuard requires vector indexing (how can this be made thread safe for Arc without a mutex?).

A strong -1 for me, sorry.

[rkjnsn]

@pythonesque

I think you misunderstand the point of this proposal. The guarded Rc proposed here can be used exactly like a normal Rc. The only difference usage-wise is that cycles get cleaned up automatically if you neglect to do it manually, allowing the presented destructor guarantee to be upheld. The destructor guarantee is the point of this proposal. Everything about Rc is to demonstrate that it can still exist and do all of the same things in a world with this guarantee.

The whole point of reference counting cycles is to be able to break them dynamically.

There is absolutely nothing in this proposal that prevents you from doing that.

You can replicate the functionality in this RFC in Rust already with a TypedArena; you shouldn't be using Rc if you know when things are going to be dropped.

This just isn't true. To quote myself from Reddit:
Reference-counted objects can contain arbitrary types allocated on the heap that are only kept around as long as there are active references. TypedArenas hold objects of a fixed type, only let you allocate during its lifetime, and free everything at once at the end. They serve completely different usecases.

This proposal doesn't change the use or operation of reference counting in any way. The only thing it does is make sure that in the case of cycles, which are not the norm, the contained objects are still cleaned up before their lifetime ends.

I want to be clear, because it seems like there is some confusion: This propsal doesn't make Rc's stay around until the guard is executed. When the strong count reaches zero, the contained object is dropped, like always. When the weak count reaches zero, the allocation is freed, like always. This proposal doesn't change that in any way.

This proposal doesn't require you to know when things will be dropped, only the maximum amount of time they could live, which you have to know anyway due to Rust's lifetime system.

This RFC also adds potentially failing operations on Deref

Rust has runtime failures all over the place. borrow_mut an already borrowed RefCell? Panic! Index an array out of bounds? Panic! Overflow an integer in a debug build? Panic!

In general, Rust asks, is this error a programming/logic error or a valid runtime condition? If it's the former, the function panics or aborts. If it's the latter, the function returns a Result. This case very clearly falls into the programming/logic error side of things: First, the programmer has to create a type capable of being part of a cycle (internally mutable Rc of the same lifetime). Second, the programmer has to actually create a cycle and fail to tear it down manually. Third, the programmer has to attempt to dereference the internal Rc during its destructor. Not only is this obviously a logic error, it's also much less likely to occur than other cases where rust panics or aborts.

How can this be made thread safe for Arc without a mutex?

First, I'll note that ScopedRc can almost certainly be implemented better. I mainly wanted to create a proof of concept implementation to show that what I was proposing was possible. However, even if it were kept exactly the same, the only time you would need a lock would be when allocating a new object in an Arc and when an Arc's reference count reached zero and it needed to be deallocated. I believe deallocation always needs an allocator lock, anyway, and allocation sometimes does, so this would be on the same order of magnitude. Cloning an Arc, dereferencing, downgrading and upgrading, et cetera would all be just fine the way they are now, with no lock needed.

You also wouldn't have to worry about race conditions during cycle collection because all external references are guaranteed to have been dropped by then due to lifetime restrictions, so the only thread with access would be the one destructing the guard.

[Ericson2314]

Big +1.

[retep998]

I'd be so happy if we could get something like this in pre-1.0

[daheise]

I'm looking at @pcwalton's comments in #1066 and trying to grasp how the competing proposals may address his concerns. It sounds like there is concensus on retaining a version of Rc that is both safe and allows cycles. I like the proposal here, but want to make sure I understand it correctly and want to know if it addresses @pcwalton's concerns.

Is this a fair very high level description of the proposal: There will be a version of Rc that will use the type system to statically disallow cycles, an AcylicRc. This version will be safe and will not not have a 'static constraint.

On the other hand, one can use another variant of Rc, say MayCycleRc, that can potentially contain cycles, and it will have a 'static constraint. I saw in the other proposal a suggestion on this MayCycleRc idea that one could drop the 'static constraint and then it would be marked as unsafe. It sounds like either of these constraints are a non-starter for @pcwalton .

I think this brings me to my question. I know everyone here wants hard guarantees about destructors being run, including me. However, I'm wondering if it would be sufficient for the compiler to issue a strong warning on today's Rc when one creates an Rc type such that it could potentially cycle. The consensus seems to be that leaking memory in Rc cycles isn't unsafe, but it is undesirable.

[Ericson2314]

@heiseda This also allows cyclic non-'static Rc. We can do this because the lifetime guarantees a point after which all nodes must be cleaned up.

[daheise]

@rkjnsn @Ericson2314 In practice, how does this differ from TypedArena?

[Ericson2314]

@heiseda if you look to comments above (#1094 (comment)) @rkjnsn just addressed this. But to summarize:

• The RC graph need not have all nodes be of the same type. Typed arena has all nodes of the same type.
• Nodes can and will still be cleaned up before the lifetime ends, just like today. Typed arena does not release anything until the lifetime ends.

[pythonesque]

I posted a similar comment on Reddit, but to summarize my current thoughts:

Firstly, I would vastly prefer if this RFC took advantage of dropck (statically avoiding the possibility of using a cyclic reference within a destructor), because as it is it's going to induce overhead like bounds checking does, where it's usually not necessary for safety (except that unlike bounds checking, the compiler won't be good at eliding it). I think this should be doable by just limiting each RcGuard to a single type; Rcs of different types can reference each other by the usual technique of defining all the RcGuards in one let tuple. Keep in mind that Rc is really competing with garbage collection, where it already probably loses; this adds the equivalent of a write barrier on every access and cycle collection, which means it definitely loses.

Secondly, there is definitely going to be substantial overhead with any proposal I can think of that works with cycles in Arc, for creating and dropping them (it will require synchronous updates of the backing vector through a mutex). This may or may not matter in practice, but it needs to be benchmarked and it will be a sizable regression in addition to other bookkeeping required (including a vector of fat pointers as long as the number of live Arc pointers).

Most importantly, the requirement for a ScopedGuard is going to mean that people will need to expose the use of Rc and Arc through their constructors (i.e. they'll need to take a ScopedGuard as a parameter) if they want to allow non-'static types to be used with them. Sometimes this will be avoidable (if they know cycles are impossible they can use the unsafe moves) but many people either will not be able to determine this, or won't want to resort to unsafe code.

My suspicion is that rather than do this, in practice most people will just bound Rc and Arc by 'static when used. Besides this being an annotation burden, it will split Rust libraries into those that are usable with Rc / Arc, and those that are usable with borrowed types. Keep in mind that many libraries use these types incidentally. As one example, if this RFC goes through without changes the channel API won't be usable with non-'static types in scoped threads; it might be possible for it to use the unsafe move constructor, but it's not immediately obvious.

One might argue that the above also applies to the ?Leak trait. The difference there is that very few types actually need to be unleakable for memory safety, but many more types are going to be non-'static. It's okay for scoped guards not to be storable in many useful types that happen to use Rc somewhere internally, less so every type with a reference (again, in practice I really don't think people are going to write extra constructors with ScopedGuards, though I'd love to be wrong).

The third point is the biggest reason I'm against this RFC. I can't see a way around it, because allowing cycles and not having a scoped guard for cycle collection fundamentally means you're accepting leaks, and I don't want types with references to turn into second-class citizens.

[Ericson2314]

@rkjnsn that is some fantastic ergonomic work. Good job!

[rkjnsn]

I updated the RFC with the revised ScopedRc design. Specifically, it now suggests replacing the current types to address composability and type proliferation concerns. ScopedRc can now be used both with and without a cycle-collection guard, with reduced allocation and deallocation overhead when not using one.

[theemathas]

A few points:

• Should it be possible to merge two ScopedRcGuards together?
• The RFC is still not clear on how to handle channels. FYI, here is how you can (currently) leak using channels.
• I think that if we're going to get rid of the old Rc, then the new one should be named Rc, not ScopedRc.

By the way, the RFC would be easier to understand and evaluate if it had the type signatures of ScopedRc, ScopedArc, and related types written down.

[aturon]

@rkjnsn, thanks for your hard work on this RFC, and everyone for the great discussion!

(Comment below copied from #1085 (comment)):

Of course, this needs to be settled prior to the 1.0 release next week, and the core team met yesterday to come to a final decision on the matter. This is truly a thorny problem with multiple reasonable paths to take, but in the end the analysis of the tradeoffs presented in Niko's blog post and the follow up represents the core team's consensus, which emerged through the discussion on this thread and others.

As such, we are going to close this RFC, to settle the matter for 1.0.

There is still discussion to be had about what to do with the scoped API (which might be best done through iteration in crates.io first). And in the future, we may want to consider making this kind of deferred computation a first-class feature.

[rkjnsn]

@aturon I made a number of changes to the RFC in response to the feedback it the posts you mention, among others, and there has been no discussion here from any of the core team regarding them, or whether and why they are still insufficient.

Specifically, I significantly reduced the overhead for unguarded Rcs, and made it easy to create unguarded Rcs for both static data and non-static acyclic data. I also posted the results of a trial run with the compiler, which demonstrated no noticeable change in compile time or memory usage. Are there other real-world, Rc-heavy workloads for which before and after benchmarks would be useful?

I put a lot of effort into specifically addressing the discussions you mention as the reason for closing this, so it's very disheartening to see it closed with no further feedback.

[rkjnsn]

@theemathas

Should it be possible to merge two ScopedRcGuards together?

No, they could only be merged if they had the exact same lifetime, in which case the programmer could just use a single ScopedRcGuard to begin with.

The RFC is still not clear on how to handle channels.

I discuss this some in this comment.

I think that if we're going to get rid of the old Rc, then the new one should be named Rc, not ScopedRc.

I agree. The RFC calls it ScopedRc to distinguish it for discussion purposes.

By the way, the RFC would be easier to understand and evaluate if it had the type signatures of ScopedRc, ScopedArc, and related types written down.

There is a sample implementation and usage linked to from the RFC, but perhaps some of the details should be added to the RFC itself.

[aturon]

@rkjnsn

I'm sorry, in the discussion in the core team yesterday the understanding was that these latest changes were covered by the follow-up blog posts and further replies in the thread there and elsewhere. Let me try to lay that out in more detail.

In particular, the follow-up post lays out concerns about integration with external reference-counting systems, and @nikomatsakis also discusses a basic disagreement about the effective mental model around 'static in particular, neither of which are addressed in these changes. There is still the question of leakage into channels and other APIs built on top of reference counting, which was raised in the original post but doesn't have a definitive answer in this RFC -- and all of the potential answers involve altering the channel API in some way. That is, the fact that a given API happens to use reference counting internally still winds up leaking through (which poses a composability and API evolution problem). And there is still an overhead imposed when Rc is used with borrowed data. I believe all of these points have been raised elsewhere (for example, in @pythonesque's comment). They are non-trivial downsides.

On the other side, as @nikomatsakis lays out in the blog post, all of these downsides have to be weighed against the problem the proposal is trying to solve. As @kballard nicely articulated, the use of RFC for cases like scoped is suspect in the first place: you do not actually use the guard to access the data it is meant to protect. When the two are tied together (as is the case in almost every other use of RAII we're aware of), leakage does not seem to pose a fundamental safety problem. More of the thinking on this topic is laid out in this comment and this one.

I imagine @nikomatsakis can give you some additional feedback and pointers, though he's away at the moment.

Finally, while we've tried to focus this debate on the merits of various proposals in abstract, it's also worth remembering that 1.0 is slated for release in one week. As we've discussed in this thread, the blog post, and elsewhere, the fundamental problem being solved here is relatively narrow, and the potential risks of the solution potentially high. Putting all of those factors together, it seems quite imprudent to delay the release at this point, as would be needed to fully carry out and evaluate a solution like the one proposed in this RFC.

[Ericson2314]

@aturon I'm with @rkjnsn here. The RFC and implementation has seen significant changes since the follow up blog post even.

Obviously there has been few others besides us pushing for this, so I can't complain if you reject it on grounds of lack of community support. But I do too feel that the reasons as given miss the mark or are out of date. I am going to address the core team's points as summarized by you in a few minutes.

Ideally at this point I'd like to see both non-static rc and scoped threads made unstable for 1.0 so we can actually verify the claims made by both sides.

[rkjnsn]

I feel like the external reference counting was addressed nicely by @glaebhoerl, with whom @nikomatsakis seemed to agree.

Different mental models regarding 'static may affect whether this RFC can be seen as complete solution as opposed to one addressing specific issues, but I think it's useful, either way. In my mind, it brings the story regarding lifetimes and destructors to a very consistent state, but regardless, it means that relying on a destructor can never lead to memory unsafety, which is with what Rust is most concerned.

I disagree that this RFC would cause additional implementation details to leak for channel. The problem with channel is the API itself, which allows leaks because it already exposes aspects of its implementation. This RFC would require the API to be adjusted to prevent leaks, but that's independent of the fact that it uses Arc under the hood. The ideal solution to this problem is to prevent the Receiver from being sent into its own channel, which, unlike Rc cycles, is never desirable.

"And there is still an overhead imposed when Rc is used with borrowed data." One part of the revision allows using Rc without the overhead of a cycle collector (as long as cycles aren't needed). Additionally, the extra overhead when using a cycle collector only occurs when an object is allocated or deallocated, and consists of a few O(1) operation. I believe that performance impact would be negligible for any real-world usage, but I would be happy to adapt some existing code if there are specific benchmarks you'd like to see.

The goal of this RFC isn't to save scoped. I am rather ambivalent about whether the new design is better or not. I do, however, think that the fact that scoped was introduced in the first place, and accepted by those who work with the language the most, shows that similar mistakes will be made again and again as long as destructors can't be relied upon for safety. If the question were just "change Rc" or "change scoped", I would agree that changing scoped would make more sense. However, the choice is not just about changing scoped but rather providing a useful guarantee for the language as a whole, and I think that is more than worth a few changes to Rc.

As for the limited timeline, this is fortunate, but understandable. While I believe the benefits of this RFC far outweigh the downsides, I understand the strong desire to release on schedule that one week is probably not enough time to fully evaluate and implement it given everyone's busy pre-1.0 schedules. Ideally, this would have come up sooner, but I'm not sure my solution would be possible without @pnkfelix's dropck work. If this RFC isn't accepted due to lack of time, what would the steps be for trying to get it included in 2.0?

[rkjnsn]

It also would be nice to have feedback from @alexcrichton, who proposed the original forget RFC and @pcwalton, regarding how this fits with his Servo needs.

[rkjnsn]

Specifically, this RFC allows both creating cycles and leaking 'static data, which seemed to be the things @pcwalton was concerned about losing.

[tshepang]

@rkjnsn now that you mention 2.0, I sometimes wish Core Team would decide on a roadmap for it soon. I think there are going to be more issues, especially since Rust is going to see more usage with 1.0. Maybe a Github label like 2.0 could be created for such future breaking changes.

An example of such a roadmap would be:

After 10 releases (over 1 year), we plan to release 2.0 which implements breaking changes. All such changes will be marked so in official docs to warn developers. This would be in addition to removing deprecated APIs.

1.0 would perhaps be the shortest major release. All others would last several years each.

[theemathas]

@aturon IMHO this usually is not a leaky abstraction (channels are a weird case: I'm still trying to figure out the solution). Most code that use ScopedRc probably would either:

• Have a clear owner, and that owner can own the ScopedRcGuard
• Have dynamic invariants, which requires unsafe
• Clearly have shared ownership, which means that the user should already know that the structure has an ScopedRc somewhere. In this case you can make the guards part of the public API
  By the way, since this RFC specifies that any panic that occurs while a destructor is in progress results in an abort, we still need a scoped redesign. However, I find the case of Vec::drain() interesting. This RFC would actually make the old implementation of Vec::drain() sound.

[theemathas]

Some solutions for channels:

• Use some strange lifetime tricks we don't have yet.
• Create a ChannelGuard type, and make creation of non-'static channels use it.
• Make creation of non-'static channels use ScopeArcGuard.
• Merge ScopedRcGuard and ScopedArcGuard in this RFC with thread::Scope from the scoped threads RFC, resulting in a single type (possibly named scope::Scope and acting similarly to the old try_finally() function). Also make channels use this type.
• Make creation of non-'static channels require a thread::Scope (I prefer this solution)

[nikomatsakis]

@rkjnsn

First, I'd like to offer an apology: I did skim over the most recent changes to the RFC, but I didn't fully appreciate the work you had done to reduce cost and offer more specialization for specific scenarios. This was clever work. I'm glad you raised an objection so that we can talk it out a bit more.

All that said, the newer draft of the RFC doesn't really change my final opinion. There are several factors here, but I think the root cause is this: Ultimately, this proposal is aimed at making unsafe code easier to reason about, which is a laudable goal. But that goal is achieved by making safe code more complicated, and I believe that this problem leaks up into surrounding APIs that will employ reference counting, as @pythonesque has noted. This is about more than the channel API (perhaps it could be refactored, I don't know); just using Rc itself has become more involved, since one must understand the scoping system. Moreover, anyone who wants to write code that uses reference counting will have to take similar measures (and they may not all be able to lean on Rc).

I have this strong feeling that simple, straight-forward reference-counting should not be unsafe, and should not require extra runtime overhead. Shared ownership is a fact of life and a very common pattern, even if less common than single ownership. We already place limitations on mutability, which is required for any form of safety, but further complicating the Rc type feels like a significant loss to me. Earlier I tried to make this point by referencing external systems; now, I agree with @glaebhoerl that those systems carry a lot of further complications, so perhaps T:'static is reasonable there in practice. But I think the principle applies also to pure Rust code and to unsafe Rust code as well, and in those cases bounds of T:'static feels bad.

As far as overhead goes, I appreciate that your scheme can sometimes rule out cycles statically, but in so doing it prohibits recursive Rc types, which are a very common case! This is even stricter than the old scheme, which required immutability, so I feel pretty sure that it will not be widely applicable in practice. For example, I don't believe the compiler could use it to accommodate its arena pointers. This implies to me that, in real-world cases, we won't be able to achieve zero-cost safety when using reference counting. That is, we will impose the runtime overhead of maintaining the linked list -- which, incidentally, proved to be substantial back in the days of @T -- as well as the ergonomic overhead of threading the scope around.

To go further, tying guarantees around leaks to a 'static bound kind of works against one of the goals of Rust's design. The idea is that lifetimes serve as a "static filter" over when a value can be accessed, but don't affect the dynamic semantics. This helps to make it possible to write code generically over all types (not just 'static types) in as many scenarios as possible. Now, sometimes it just can't be done, as with thread::spawn, but I think that be careful when adding a new category of thing that is now unsafe if borrowed pointers are around. Each such change causes a composability hit, no matter how clever the API, and will likely lead to further implications down the line.

All this is not to say that I don't like your proposal. In fact, I like it quite a bit! But I don't think it's a pure win: it imposes real costs. In short, I think there are good arguments in favor of the status quo, and it has the advantage of being the devil we know. We have a lot of experience with the current system and a good notion of how well it scales.

Now, the elephant in the room is obviously the 1.0 release. For the most part, I've been trying to think about this problem from first principles and decide what's best for Rust as a language. If we have to delay the release, so be it. But obviously we have very publicly announced that we will release 1.0 on May 15th, and if we decide we want to make changes to Rc, we're going to have to back away from that date. We will also have to weaken the coverage of the stable API, because we need time to iterate and gain experience with whatever replaces Rc. I think slipping and limiting the release will do harm to the project, so I want to be sure the change is worth it. Getting a new language off the ground is hard enough, especially a new language that asks people to change how they think. Giving the impression that it will never ship feels very risky to me (not to mention the risks of making Rc more complex).

Finally, with regard to your point that people will write unsafe code that relies on destructors even though they shouldn't, I certainly agree that will probably happen. But I also think people are going to create leaks, even if we say that they are unsafe (along with a lot of other bugs to boot). In the end, we have to publish clear guidelines on unsafe code (working on it) and make progress on tools to help people validate their unsafe code (thinking about it, at least).

[aturon]

@rkjnsn

@nikomatsakis is going to reply more fully, but I wanted to elaborate a bit on some of the points I raised as well.

I feel like the external reference counting was addressed nicely by @glaebhoerl, with whom @nikomatsakis seemed to agree.

I'll let @nikomatsakis follow up on this, but I think the basic point remains: with this RFC, it will simply not be possible to provide or interface to "simple" reference counting (without cycle detection or prevention) that is generic over borrowed data.

Different mental models regarding 'static may affect whether this RFC can be seen as complete solution as opposed to one addressing specific issues, but I think it's useful, either way. In my mind, it brings the story regarding lifetimes and destructors to a very consistent state, but regardless, it means that relying on a destructor can never lead to memory unsafety, which is with what Rust is most concerned.

I agree that the mental model point can be reasonably argued in a few directions here, but want to be clear that in all schemes, memory unsafety can only arise through explicit use of unsafe code; this is all about the assumptions you can make in unsafe code. I believe that @kballard's point about the typical RAII pattern is a crucial one: the vast majority of the time, when RAII is used, the guard is actually a guard: you have to go through it to access the data. For such cases, experience shows that leakage poses little risk. (I've pointed to this comment before, but @nikomatsakis offers a nice analysis here).

There's broad agreement that providing stronger guarantees and a stronger mental model, all else aside, is great. But it has to be balanced against the costs.

I disagree that this RFC would cause additional implementation details to leak for channel. The problem with channel is the API itself, which allows leaks because it already exposes aspects of its implementation. This RFC would require the API to be adjusted to prevent leaks, but that's independent of the fact that it uses Arc under the hood. The ideal solution to this problem is to prevent the Receiver from being sent into its own channel, which, unlike Rc cycles, is never desirable.

Whether you consider them implementation details or not, the fact is, this RFC would force the channel API to change, and would likely have impact on other APIs as well. Certainly anything that uses Rc internally would have to deal with the potential for cycles somehow, which may require difficult API design tradeoffs (as is already the case for channels).

"And there is still an overhead imposed when Rc is used with borrowed data." One part of the revision allows using Rc without the overhead of a cycle collector (as long as cycles aren't needed). Additionally, the extra overhead when using a cycle collector only occurs when an object is allocated or deallocated, and consists of a few O(1) operation. I believe that performance impact would be negligible for any real-world usage, but I would be happy to adapt some existing code if there are specific benchmarks you'd like to see.

The problem is with the phrase "as long as cycles aren't needed" -- it's not that simple. The proposed API requires you to "prove" the lack of cycles by means of dropck-enforced markers, but that is inherently tied to the stack structure of the program. It's not at all clear how many uses of Rc will fall into this category; we have no experience with it. Making static cycle prevention practical -- making it work at scale, without a lot of additional complexity -- is an open research question, and something that Rust has already explored (and moved away from).

I think it's fair to say that in this scheme, overhead will be required at least some of the time, and we don't know where the line is. Furthermore, to escape overhead, you have to program with markers, which is an additional programming burden we have little experience with. We have been pushing Rust toward lower and lower-level safe systems programming, and part of that push has been to guarantee safety without overhead. Reference counting is a basic part of that domain, and imposing overhead and/or extra programming complexity feels like a step backwards -- the benefits would need to be very strong indeed.

The goal of this RFC isn't to save scoped. I am rather ambivalent about whether the new design is better or not. I do, however, think that the fact that scoped was introduced in the first place, and accepted by those who work with the language the most, shows that similar mistakes will be made again and again as long as destructors can't be relied upon for safety. If the question were just "change Rc" or "change scoped", I would agree that changing scoped would make more sense. However, the choice is not just about changing scoped but rather providing a useful guarantee for the language as a whole, and I think that is more than worth a few changes to Rc.

I'm very sympathetic to this point, but it's a global cost/benefit analysis. In my opinion, the style of RAII usage that leads to problems with leakage is fairly niche. Bugs will happen no matter what we do. So we have to weigh the likelihood and severity of a given class of bugs against the complexity cost imposed in trying to rule it out. FWIW, our experience elsewhere in the standard library over time has shown that complicating basic APIs to try to force programmers to account for corner cases is often a losing proposition. Further, the API cost here isn't fully known -- certainly Rc and Arc become much more complex, and channels have to change, but what other APIs further have to change to avoid leakage? And there's still the potential for unsafe code to accidentally introduce the possibility of leakage.

As for the limited timeline, this is fortunate, but understandable. While I believe the benefits of this RFC far outweigh the downsides, I understand the strong desire to release on schedule that one week is probably not enough time to fully evaluate and implement it given everyone's busy pre-1.0 schedules. Ideally, this would have come up sooner, but I'm not sure my solution would be possible without @pnkfelix's dropck work. If this RFC isn't accepted due to lack of time, what would the steps be for trying to get it included in 2.0?

Yes, I feel that even if we could devote many people evaluating this full time, a week is far too little time to truly understand the fallout of a fundamental change to the guarantees that unsafe code must provide, or even to be sure that the proposal completely works. Frankly, I'm still worried that we have leaks elsewhere in the language or library that we haven't discovered yet.

Regarding the 2.0 question, it's not at all clear what the timeline or impetus for such a release would be, given that we haven't put out our initial release yet; I don't want to speculate on that. I think we need to make a decision about 1.0 now, and then let the dust settle for a while.

[wycats]

There's something I'm trying to understand about the details here.

In particular, when you have a Foo<'a>, there seems to be a desire to be able to say that the foo was destroyed the extent of 'a is finished. In other words, there is a desire to tie the 'a lifetime in the type with the total lifetime (no pun intended) of the value.

But since subtyping allows us to shorten lifetimes arbitrarily, doesn't that violate the idea that the structs containing them are dropped by the time the 'a scope is exited? In other words, if you have a Foo<'long> where 'long is supposed to tie the lifetime of the value to 'long, and you can arbitrarily create Foo<'short>, how can Foo<'short> express the same relationship?

[rkjnsn]

@aturon @nikomatsakis Thank you both for your detailed responses. That helps me understand your reasoning a lot better.

[theemathas]

@nikomatsakis

The idea is that lifetimes serve as a "static filter" over when a value can be accessed, but don't affect the dynamic semantics. This helps to make it possible to write code generically over all types (not just 'static types) in as many scenarios as possible. Now, sometimes it just can't be done, as with thread::spawn

I disagree with this argument.

• The only reason this RFC gives different semantics to 'static is optimization using the fact that 'static data cannot have Rc cycles. If we don't care about performance, then we could make ScopedRc::unguarded() call STATIC_GUARD.new_rc()
• It is possible to implement thread::spawn() in terms of the proposed thread::scoped() if we have a STATIC_THREAD_SCOPE

Now, the elephant in the room is obviously the 1.0 release.

I believe that's the only problem with this RFC (which is sad).

[theemathas]

@aturon

The problem is with the phrase "as long as cycles aren't needed" -- it's not that simple. The proposed API requires you to "prove" the lack of cycles by means of dropck-enforced markers, but that is inherently tied to the stack structure of the program. It's not at all clear how many uses of Rc will fall into this category; we have no experience with it.

I believe that the proposed API does not require you to prove the lack of cycles. You either

• Put 'static data in it, when overhead is reduced due to the fact that you don't need to check for cycles.
• Put any data in it, and collect cycles when you drop the guard.

[aturon]

@theemathas

The problem is with the phrase "as long as cycles aren't needed" -- it's not that simple. The proposed API requires you to "prove" the lack of cycles by means of dropck-enforced markers, but that is inherently tied to the stack structure of the program. It's not at all clear how many uses of Rc will fall into this category; we have no experience with it.

I believe that the proposed API does not require you to prove the lack of cycles. You either

• Put 'static data in it, when overhead is reduced due to the fact that you don't need to check for cycles.
• Put any data in it, and collect cycles when you drop the guard.

The context here is for cases where you have borrowed data (so 'static isn't an option). In that case, with this RFC, you either:

• Pay in runtime overhead (with cycle collection)
• Prove the absence of cycles via the marker

It's unclear how far the no cycle marker goes in terms of expressiveness, but it certainly imposes some new programming complexity. So the point here is just that, when working with Rc over borrowed data, no matter which route you take, you are losing something with this proposal -- it has a cost.

[nikomatsakis]

@theemathas

The only reason this RFC gives different semantics to 'static is optimization using the fact that 'static data cannot have Rc cycles. If we don't care about performance, then we could make ScopedRc::unguarded() call STATIC_GUARD.new_rc()

I think I was not quite clear in explaining my point; but, also, what you wrote here is not strictly correct. You state the 'static data cannot have Rc cycles -- but this is not the case. Rather, it is permitted (under this RFC) for 'static data to have Rc cycles. And this was what I was getting at when I said that the "dynamic semantics" (not the best term) were affected by lifetimes -- not that we execute different code for 'static data, but rather that we permit leaks and cycles in those cases, which means that it's easier to build abstractions that use Rc for T if T:'static than otherwise, because more operations are legal on such types. So whether or not T:'a holds affects more than whether T can be used during 'a, it affects how it can be used.

[theemathas]

@nikomatsakis I've always assumed that all abstractions could take a ScopedRcGuard, and let the user pass in STATIC_GUARD as required. However, seems like you feel that is too complex. I guess I am underestimating the complexity (which is subjective any way, so I guess I should stop argue now)

@aturon I sort of forgot that rust is about "zero cost abstractions", after all. I thought that the overhead is going to be low, but maybe that's not low enough.

Sorry for beating the dead horse.

[rkjnsn]

@aturon @nikomatsakis

Thanks again for taking time to respond. Since this obviously can't be part of 1.0 at this point, I'm going to take a little break from it, but I would definitely like to refine the design a bit to see if it would be practical as a future addition. I think there are a few language features that could help, here.

One would be a way to specify additional dropck-related restrictions for a type in where clauses of generic methods. Specifically, it would be nice to be able to require not only that T is valid for 'a, but also that 'a is not visible from T's destructor. This would allow dereferencing a collected ScopedRc to be statically prevented, removing both the need for a runtime check in deref and the possibility of an abort. This would address one of @pythonesque's concerns with this RFC, and also make it possible for objects contained in an untyped Arena to safely reference each other like they can now do in a TypedArena.

Another useful feature would be syntax to specify that a given T strictly outlives 'a instead of allowing them to have the same lifetime. This would simplify NoCycleMarker. Instead of having to anchor it somewhere and have the created ScopedRcs borrow it, you could just have a zero-size, copyable marker that could be stored in any object. This would provide an easy solution to channel and similar APIs: just include a NoCycleMarker in the returned types, adjust the lifetimes accordingly, and you'd be guaranteed that you could never send Receiver into a Sender without requiring any run-time overhead or additional unsafe code.

Are there other concerns that I should try to address? Any good real-world heavy users of Rc I should use for testing and benchmarking?

[aturon]

@rkjnsn Just wanted to say -- things are very hectic on my end right now, but I will try to give some feedback on this comment soon.

[nikomatsakis]

@rkjnsn

Indeed, I had started a reply to this comment and then it got list in the shuffle. Let me note down some quick thoughts on your comments.

First, the additional dropck restrictions you mentioned are things that @pnkfelix and I have talked about from time to time as well. This relates to an (in-progress) RFC I am working on buttoning up some aspects of the type system oriented around guaranteeing that whenever you have access to data in the lifetime 'a, the lifetime 'a is in scope. One way that you could imagine a destructor declaring that it cannot access the data in 'a would be to have it "opt out" of those requirements, which could in principle make it clear that it cannot use that data.

Second, adding the ability to have a "strict outlives" relation does seem useful, but I am wary, because I think it could potentially impact other aspects of the language design. For example, we assume that today, during code generation, that we can erase lifetimes without affecting trait selection -- but if one could have a strict outlives relation, that might not always be true (it'll depend on how things like coherence and specialization treat regions as well). In general, this points to the need for more formal modeling, which is definitely something that is in progress.

I'll be honest, it's unclear to me whether this decision will be reversible in a practical sense. Even if we were to issue Rust 2.0, I could imagine that it is either impractical to deprecate Rc (would break too much code) or that there is too much unsafe code which relies on the possibility of leaks (i.e., integrating with other ref-counting systems etc). But, assuming that this were not to be the case, I think the primary thing we would want to overcome is the "API burden" question.

To that end, one obvious starting place is to look at abstractions like channels and try to modify them so as to ensure that they cannot leak (some thoughts were already put in place on this thread, iirc). Another thought that I have had concerns integrating with other kinds of shared ownership beyond Rc, such as Gc. The current "leaks are ok" design enables Rc and Box to be used equivalently, if you don't need mutation. But Gc has complex interaction with destructors as well, for different reasons. In the past, we've assumed that Gc would have a 'static bound, but that's not particularly satisfactory for all the reasons that have been enumerated in this thread; it also fails to address the critiques of Gc and destructors that arise from other languages, which don't even have lifetimes. An alternative approach might be to have an implicit scope that gets passed around, which could be used to control when destructors run (and in turn to guarantee lifetimes in the same way that you have done here). In that case, I could imagine that RcScope and Gc become somewhat interchangable, and a lot of code that wishes to build on shared ownership as an abstraction winds up implicitly threading scopes through anyhow, just to be interoperable with Gc and Rc. This might argue for deprecating Rc and trying to transition to RcScope as the sole approach (obviously this would be a Rust 2.0 sort of change).

Finally, I just want to note that even if we never fully deprecated leaks, I think that RcScoped has independent value. Nobody wants their code to leak. So making it available on crates.io (and continuing to improve it!) seems like an obvious first step whatever happens! It'd be great if we could find effective ways to guarantee absence of leaks even for 'static types. (If nothing else, immutability would be useful here, though we know there are cases where it's too strong.)

[Ericson2314]

@nikomatsakis Interesting stuff, thanks for writing it!

• I think RcScoped in cargo could indeed be a useful component in the push towards custom allocators and GC.
• As a general principle (as opposed to regarding RcScoped in particular), I'd hope no amount of compatibility breakage would be automatically off the table for 2.0 iff the benefits to compensate were sufficiently large and guaranteed, but I suppose we'll cross that bridge when we get there.