Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for Arc/Rc extras #28356

Closed
alexcrichton opened this issue Sep 11, 2015 · 31 comments
Closed

Tracking issue for Arc/Rc extras #28356

alexcrichton opened this issue Sep 11, 2015 · 31 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

This is a tracking issue for the arc_counts, rc_counts, and rc_would_unwrap features. The counting functions have been around for awhile but would_unwrap was added recently.

cc #27718, more discussion there.

@alexcrichton alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Sep 11, 2015
@timcharper
Copy link

@alexcrichton I think there may be a good use for strong_count in detecting un-fulfillable promises. Although, I am struggling with it, because most cases I think of are racy.

If you try and get the value of a promise synchronously, and there is only one instance of the promise, then you can fail the promise ahead of time because either nobody is listening, or nobody is fulfilling.

promise

See the discussion here: viperscape/rust-promise#3

@timcharper
Copy link

The more I think about this, I think the more your point about hard_count being finicky is corroborated. This case is racey and complex. It'd be better / more deterministic to just have the promise register itself to listen for a thread panic, and just fail if it does.

@alexcrichton
Copy link
Member Author

@timcharper yeah in general the methods on Arc are inherently racy, but there's a point that can be made where there's external synchronization in play so the raciness doesn't actually happen in some specific scenarios (in which case these may be desired)

@jethrogb
Copy link
Contributor

I rewrote my usecase for strong_count to work with Option<Arc<_>> and Arc::try_unwrap so I no longer actually need this.

@shahn
Copy link
Contributor

shahn commented Dec 19, 2015

If this gets stabilized, these APIs should get added to the respective Weak as well I'm thinking

@johalun
Copy link

johalun commented Dec 24, 2015

What is planned for arc_counts? I would like to use it because I need to do ffi call to c library when the last Arc pointer is dropped but don't wanna start using it if it might disappear soon.. Maybe a different approach would be to do the call when the inner structure is being dropped, that would definitely be when the last pointer is dropped because is has only one owner..

@ticki
Copy link
Contributor

ticki commented Dec 28, 2015

May I ask: What is the point of Arc and Rc keeping the weak reference count? They should be irrelevant to them.

@Gankra
Copy link
Contributor

Gankra commented Dec 28, 2015

@ticki I don't understand the question. Rc/Weak are dual types that have different interpretations of the same memory. Individual Rc/Weaks do not store a count. Rather, they all share RcBoxes to coordinate these counts.

@ticki
Copy link
Contributor

ticki commented Dec 28, 2015

I don't understand the question. Rc/Weak are dual types that have different interpretations of the same memory. Individual Rc/Weaks do not store a count. Rather, they all share RcBoxes to coordinate these counts.

Yeah, that's right. They're just pointers to the actual data plus a weak and a strong reference count. What I ask is: why do they keep track of the weak reference count? For example, C++ and Obj-C do not count the weak references.

@timcharper
Copy link

If I were to venture a guess, strong reference counts are used to know when the ARC contained data should be freed, weak reference counts are used to know when the ARC container should be freed.

I'm surprised to learn that C++ / Obj-C don't do this (at least, under the hood). What data structure does the weak reference query to learn if the memory has been freed?

@ticki
Copy link
Contributor

ticki commented Dec 28, 2015

In fact, I was wrong about C++ shared_ptr:

The control block is a dynamically-allocated object that holds:

  • either a pointer to the managed object or the managed object itself;
  • the deleter (type-erased);
  • The allocator (type-erased);
  • the number of shared_ptrs that own the managed object;
  • the number of weak_ptrs that refer to the managed object.

(emphasis mine)

Source: http://en.cppreference.com/w/cpp/memory/shared_ptr

@Gankra
Copy link
Contributor

Gankra commented Dec 29, 2015

Obj-C strategy is explained here: https://mikeash.com/pyblog/friday-qa-2010-07-16-zeroing-weak-references-in-objective-c.html

Basically: every class instance has a set of "locations of weak pointers". When its strong count hits 0, it traverses the set, nulls out those pointers, and then frees all of its allocations.

This can't be implemented in Rust, because it requires weak pointers to have significant locations in memory. Would necessitate overloading move (never gonna happen), or runtime support (unlikely to happen).

That said, @pnkfelix has been working on the notion of stack introspection hooks for GC's, which could maybe accommodate this use-case..? Not something we would retrofit Arc/Rc to use, but something someone could do with their own custom types.

@jminer
Copy link

jminer commented Dec 30, 2015

Does it look like either rc_counts or rc_would_unwrap will be stablilized? I'm wrapping a C library and have a place I'd like to test strong_count == 1 or use would_unwrap, and similarly to @SimonSapin's usecase, I can't use try_unwrap. The check is in drop of a wrapper struct, where I only have &mut self (I need to free some memory when the last reference is dropped). I can probably add another struct to wrap the contents of the Rc, but the code won't be as clear.

@SimonSapin
Copy link
Contributor

Another use case for strong_count:

The heapsize crate traces through things and counts heap-allocated memory. Doing so with Arc<T> or Rc<T> is tricky: servo/heapsize#37. One idea is to divide by the strong reference count: consider that each reference "owns" a fraction of the memory.

@pnkfelix
Copy link
Member

Another use case for strong_count:

The heapsize crate traces through things and counts heap-allocated memory. Doing so with Arc or Rc is tricky: servo/heapsize#37. One idea is to divide by the strong reference count: consider that each reference "owns" a fraction of the memory.

If another reader sees this and like me thinks "won't the need to handle cycles provide a separate place to represent this information?" then you should read the linked issue. (Quick answer: maybe yes, but it sounds like it doesn't currently handle cycles well anyway, and the comment thread has some interesting alternative ideas beyond a separate map of seen addresses.)

@SimonSapin
Copy link
Contributor

@pnkfelix indeed. One of the alternative idea is to return 0 if the strong reference count is more than 1, and that would also need stabilizing one of the methods covered in this issue.

SimonSapin added a commit to servo/heapsize that referenced this issue Mar 6, 2016
This crate has always given a lower bound of the actual heap memory usage:
we use `#[ignore_heap_size_of]` when not doing it is not practical.

This fix #37 with one of the ideas given in the discussion there:
return an accurate result when the reference count is 1
(and ownership is in fact not shared), or zero otherwise.

However, since APIs to access reference counts are not `#[stable]` yet
(see rust-lang/rust#28356),
so always return zero when unstable APIs are not enabled.
@ahicks92
Copy link
Contributor

These may be useful for debugging. I don't have a concrete use case at the moment. Somehow I've managed to avoid shared ownership thus far.
However, I could see it as potentially beneficial in the future if I had to track down a reference cycle or figure out why something isn't dying. I do agree that the use in actual programs is less clear, but these are maybe worth having just so you can print them.

@ticki
Copy link
Contributor

ticki commented Apr 13, 2016

@camlorn Cycle detection in debug mode could be awesome actually.

@ahicks92
Copy link
Contributor

Agreed. I thought of it as well. But this didn't seem like the place to suggest it and in truth that sounds like it would probably be very complicated and RFC-ish anyway.
Printing the counts in debug is admittedly much worse in terms of what we have, but having these as unstable and/or killing them feels like a step backward in this department. Though perhaps only a small one.
Still, cycle detection would be a major win over C++, do that now.

@DemiMarie
Copy link
Contributor

Actually, cycle detection in could be useful for far more than just debugging: one might be able to use it to implement a cycle collector a la Python that not only detected cycles, but freed the ones that were no longer reachable.

@ahicks92
Copy link
Contributor

ahicks92 commented Aug 5, 2016

@DemiMarie
That would be unsafe. Create a cycle that is accessible from the stack but for which there is no Rc on the stack. Watch your program die.

You'd also need special compiler machinery for tracking the accessibility of stuff from the stack, and at that point we might as well just add a full GC.

@SimonSapin
Copy link
Contributor

The remaining unstable methods covered here are:

  • Arc::strong_count
  • Arc::weak_count
  • Rc::strong_count
  • Rc::weak_count
  • Rc::would_unwrap
  • Rc::is_unique

They have been #[unstable] for some time now, and I the discussion hasn’t seen new arguments for a while. Based on reading this thread and #27718, the concerns that I know of are:

  • The meaning of is_unique is not entirely obvious based only on its name, in presence of two (weak and strong) counts.
  • Use cases for is_unique and would_unwrap are niche.
  • Arc::*_count is easy to misuse: in the time between calling one of these methods and acting on the result, another thread might have done something to change a count and make that result out of date.

To counter-balance that:

  • I haven’t seen a real concern against Rc::*_count, only being conservative on principle when stabilizing things in std.
  • Arc::*_count can be used correctly in some cases (such as with external synchronization). And they just return integers, so they’re not too much of a footgun by themselves. To actually have a data race when misusing them, you’d have to also do something else that requires unsafe.
  • At least in my case, the use case for would_unwrap, however niche, was enough motivation for forking the std::rc module.
  • is_unique and would_unwrap are very easy to implement on top of strong_count and weak_count.

Given all this, I’d like to propose FCP to stabilize all four *_count methods, and deprecate is_unique and would_unwrap.

@alexcrichton
Copy link
Member Author

@rfcbot fcp merge

Thanks for the summary @SimonSapin! Sounds reasonable to me to stabilize the count methods and deprecate the others. We've already committed to reference counting and weak pointers for both Rc and Arc so exposing them seems harmless!

@glaebhoerl
Copy link
Contributor

Let's at least strongly emphasize the concurrency hazards in the Arc methods' documentation.

@SimonSapin
Copy link
Contributor

@glaebhoerl I’ve opened #37652, what do you think?

@glaebhoerl
Copy link
Contributor

LGTM :)

steveklabnik added a commit to steveklabnik/rust that referenced this issue Nov 8, 2016
…chton

More proeminent warning in Arc::{strong,weak}_count docs.

CC rust-lang#28356 (comment)
@rfcbot
Copy link

rfcbot commented Nov 10, 2016

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot
Copy link

rfcbot commented Nov 14, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

@alexcrichton alexcrichton added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Nov 14, 2016
@BurntSushi
Copy link
Member

@alexcrichton I think you need to tell rfc bot that you applied the label?

@alexcrichton
Copy link
Member Author

@BurntSushi hm I think rfcbot may be missing out on FCP endings... In any case we'll want to leave this until the near the end of the cycle regardless.

@rfcbot
Copy link

rfcbot commented Nov 24, 2016

The final comment period is now complete.

bors added a commit that referenced this issue Dec 18, 2016
Library stabilizations/deprecations for 1.15 release

Stabilized:

- `std::iter::Iterator::{min_by, max_by}`
- `std::os::*::fs::FileExt`
- `std::sync::atomic::Atomic*::{get_mut, into_inner}`
- `std::vec::IntoIter::{as_slice, as_mut_slice}`
- `std::sync::mpsc::Receiver::try_iter`
- `std::os::unix::process::CommandExt::before_exec`
- `std::rc::Rc::{strong_count, weak_count}`
- `std::sync::Arc::{strong_count, weak_count}`
- `std::char::{encode_utf8, encode_utf16}`
- `std::cell::Ref::clone`
- `std::io::Take::into_inner`

Deprecated:

- `std::rc::Rc::{would_unwrap, is_unique}`
- `std::cell::RefCell::borrow_state`

Closes #23755
Closes #27733
Closes #27746
Closes #27784
Closes #28356
Closes #31398
Closes #34931
Closes #35601
Closes #35603
Closes #35918
Closes #36105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests