Add {Arc, Rc}::{try_}into_unique

[cramertj]

Implementation of rust-lang/libs-team#608
Part of #112566

Note the -> Result return value difference from the ACP. I left this comment just now asking for the original motivation to use Option.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[lukaslueg]

personal drive-by comment: This signature has the same problem as Arc::try_unwrap() -> Result as compared to Arc::into_inner() -> Option: If two threads call into_unique at the same time without dealing with the error case (into_unique().ok()), they could both determine that they both are not the unique owners, and both (safely) discard the inner value. In order for into_unique to be used from multiple threads, all threads must always put the returned err-value back to the place where it came from, or it might get silently lost, because every thread thinks that some other thread now owns it...

[cramertj]

Rc isn't Send or Sync. Can you clarify how this method would be called from multiple threads?

[cramertj]

Rc isn't Send or Sync. Can you clarify how this method would be called from multiple threads?

[lukaslueg]

My bad, same comment, for the Arc implementation below, not Rc

[cramertj]

Ah, okay. So you're not concerned that the implementation is incorrect, but you are concerned that two different callers could additionally assume (incorrectly) that they are not the last holder of an Arc after failing to call into_unique?

That is, this situation described in the docs of Arc::try_unwrap:

It is strongly recommended to use Arc::into_inner instead if you don’t keep the Arc in the Err case. Immediately dropping the Err-value, as the expression Arc::try_unwrap(this).ok() does, can cause the strong count to drop to zero and the inner value of the Arc to be dropped. For instance, if two threads execute such an expression in parallel, there is a race condition without the possibility of unsafety: The threads could first both check whether they own the last instance in Arc::try_unwrap, determine that they both do not, and then both discard and drop their instance in the call to ok. In this scenario, the value inside the Arc is safely destroyed by exactly one of the threads, but neither thread will ever be able to use the value.

This is a surprising comment to me. Even using the non-Result-based interface, the same race would occur if any holder of the Arc is dropping it by calling something other than into_unique. In order for this guarantee to work out (at least one caller gets the unique value) all instances of the Arc must be dropped using into_unique.

The -> Option version seems like a potentially useful interface to provide, but IMO it feels like much more of an edge-case than the more usual "try to mutate this Arc in-place as an optimization, allowing for the concurrent existence of non-upgradable Weak pointers". I would've called the Option version something like unique_or_discard, perhaps?

Perhaps we should align with the existing API by using into_unique -> Option and try_into_unique -> Result?

[lukaslueg]

Ah, okay. So you're not concerned that the implementation is incorrect, but you are concerned that two different callers could additionally assume (incorrectly) that they are not the last holder of an Arc after failing to call into_unique?

Yes, my comment was purely concerned with the potential TOCTOU-pitfall when using this API. There is an unavoidable gap that allows both threads to fail in into_unique, and then the inner value to be dropped entirely. Maybe a simple doc-example will do to point this out. After all, the -> Option-variant has exactly the same pitfall, which is unavoidable.

[cramertj]

I've split this functionality into two functions, into_unique and try_into_unique.