Stabilize weak pointers on Rc and Arc, as well as the meaty uniqueness methods #27760
Conversation
Gankra
added
the
T-libs-api
|
One thing that's a bit questionable: |
|
Isn't the reason this method was omitted, the fact that it cannot be implemented safely? |
|
@Diggsey Aha, good point! (I wasn't thinking about it too much, just did a quick hackjob) |
Gankra
force-pushed
the
weaknique
branch
2 times, most recently
from
8acbe27 to
58a51dd
Compare
|
Oh, another minor issue that was present even before, and probably applies to Rc too: it's still possible to overflow both refcounts. The strong refcount can be overflowed by repeatedly cloning a Weak, and then upgrading it, forgetting the Arc it returns. The weak refcount can be overflowed by repeatedly downgrading and Arc and forgetting the Weak it returns. The fix should be straightforward. |
Gankra
force-pushed
the
weaknique
branch
2 times, most recently
from
5cb8da8 to
ecec02e
Compare
|
Rc is protected because everything correctly calls inc_strong and inc_weak -- but Arc is more adhoc and definitely still has holes as you describe. |
|
I think those holes should be addressed in another PR, though -- its quite orthogonal. |
|
It's definitely possible to implement try_unwrap correctly... |
|
@eefriedman Ah yes, that seems like the right code (weak is prevented from racily bypassing this via its CAS loop in upgrade). impl fixed. I think it should be stabilized with the rest of these methods -- there's no fundamental reason to keep it separately unstable. |
| #[stable(feature = "rc_unique", since="1.4.0")] | ||
| pub fn try_unwrap(self) -> Result<T, Arc<T>> { | ||
| // See `drop` for impl details | ||
| self.inner().strong.compare_and_swap(1, 0, Acquire) != 1 { return Err(self) } |
There was a problem hiding this comment.
Missing "if"?
Also, the atomic::fence call is redundant... or did you mean to use a Relaxed compare_and_swap?
There was a problem hiding this comment.
@eefriedman I was mimicking drop -- but now I'm not sure the Acquire is right for this CAS. iirc it's Release in drop because of some specific clause in the C11 memory model. I'm just going to punt on getting this right until @aturon chimes in.
Gankra
force-pushed
the
weaknique
branch
2 times, most recently
from
0db83d4 to
354be75
Compare
| #[stable(feature = "rc_unique", since="1.4.0")] | ||
| pub fn try_unwrap(self) -> Result<T, Arc<T>> { | ||
| // See `drop` for impl details | ||
| if self.inner().strong.compare_and_swap(1, 0, Acquire) != 1 { return Err(self) } |
There was a problem hiding this comment.
To match drop, shouldn't this be Release?
Also, can you expand on the "magic" comment below? This code is super tricky already and I'd hate to make it more difficult to understand...
|
This fixes #24789. (The PR message should include that so that it's auto-closed.) |
|
Also #27718 now that you mention it! |
|
Although this is a big step for #27718, I don’t think it should completely close it as there are unstable items left. I think stabilization tracking issues should only be closed when all relevant items are either stabilized or deprecated. |
|
☔ The latest upstream changes (presumably #27684) made this pull request unmergeable. Please resolve the merge conflicts. |
| /// ``` | ||
| #[inline] | ||
| #[stable(feature = "rc_unique", since="1.4.0")] | ||
| pub fn try_unwrap(self) -> Result<T, Arc<T>> { |
There was a problem hiding this comment.
This should not take self, yes?
There was a problem hiding this comment.
It should. When this is the only strong reference T is moved out of Arc<T> so this needs to take ownership of the reference. (Err(Arc<T>) is given "back" untouched if there are other strong references.)
There was a problem hiding this comment.
I meant that it should have the signature pub fn try_unwrap(this: Arc<T>) -> Result<T, Arc<T>> rather than pub fn try_unwrap(self) -> Result<T, Arc<T>>.
|
Closing pending rework for FCP. Will post new PR soon. |
The community has spoken: weak pointers are highly desirable!
Also Rc and Arc are kinda busted for recursive datastructures without these uniqueness methods!
The "read only" uniqueness introspection is punted on as neither critical nor obviously super useful (particularly for Arc, where such methods are inherently racy). Though they are harmless from a maintenance perspective.
Note that make_unique has been renamed to
make_mutfor symmery withget_mut.This addresses the bulk of #27718
r? @aturon on the implementation of
Arc::try_unwrapwhich I had to add since it was accidentally omitted in the past. Largely just derived fromdropon the assumption thattry_unwrapis "basically" just Drop.