Arc locking

[SoniEx2]

These are low-level primitives, which means this doesn't do anything on its own, but may enable third-party crates to provide safe wrappers around get_mut_unchecked. For example:

struct Invariant<T: ?Sized>(UnsafeCell<T>);
impl<T: ?Sized> Invariant<T> {
  fn with_mut(self: &mut Arc<Self>, f: impl FnOnce(&mut T)) {
    if !Arc::lock_strong_count(self) {
      panic!("Arc in use");
    }
    f(unsafe { Arc::get_mut_unchecked(self) }.0.get_mut())
    Arc::unlock_strong_count(self);
  }
}

Another possible wrapper would be: this one is unsound, see https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=a70acf048225b351d6620c226661e8d3 (found by @SkiFire13 )

struct Static<T: ?Sized + 'static>(T);
impl<T: ?Sized + 'static> Static<T> {
  fn with_mut(self: &mut Arc<Self>, f: impl FnOnce(&mut T)) {
    if !Arc::lock_strong_count(self) {
      panic!("Arc in use");
    }
    f(unsafe { Arc::get_mut_unchecked(self) }.0)
    Arc::unlock_strong_count(self);
  }
}

These can be compared to new_cyclic and are arguably just an extension of existing get_mut semantics. In particular, they allow safely mutating an owning Arc while there are Weaks around, by preventing them from being upgraded. Note that this condition alone is necessary, but not sufficient: Arc is covariant by default, so these wrappers are in fact necessary, because the following example would blow up otherwise:

let x: Arc<&'static str> = Arc::new("foo");
let y = Arc::downgrade(&x);
make_fuckup(x, &String::new("bar")); // transfers ownership of the only strong ref, which hypothetically is safe to mutate, but make_fuckup takes it as the lifetime of the &String!
y.upgrade(); // uh oh we now have a dangling &'static str

This is trivially a compile-time error when using Arc<Static<T>> or Arc<Invariant<T>>.

Anyway, mostly throwing this here to hopefully get a perf run on it more than anything. This adds an extra atomic load on clone.

Also note that dropping a locked Arc would lead to an abort when upgrading a Weak, or a leak otherwise, and make_mut would also make it leak. These are all safe so there's no reason to make locking/unlocking unsafe. get_mut also fails, by design. strong_count will return 0 on locked Arcs and the documentation should be updated to reflect that, or otherwise massaged, if this feature is decided to be added. (We'd prefer it to return 0, similar to how Weak::strong_count returns 0 when Arc::new_cyclic is used.)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

[Aaron1011]

This does not interact correctly with Drop - attempting to drop a 'locked' Arc will cause the strong count to underflow.

[SoniEx2]

@Aaron1011 This does interact correctly with Drop.

[Aaron1011]

It does not. Arc::drop always subtracts 1 from the strong count, and checks if the previous count was equal to 1:

rust/library/alloc/src/sync.rs

Lines 1572 to 1574 in aa8f27b

	if self.inner().strong.fetch_sub(1, Release) != 1 {
	return;
	}

A 'locked' Arc will have a strong count of 0, causing an underflow.

[SoniEx2]

@Aaron1011 And, as it turns out, that's safe. Read the PR description again.

[Aaron1011]

I didn't say anything about this being unsafe - using this API can cause a memory leak, which is not documented anywhere.

[SoniEx2]

Ahh. So we should have something like "Note that failing to unlock the Arc will cause a memory leak and may lead to an abort."?

[SkiFire13]

This doesn't need to be here since there's no way you can access an Arc while it is locked:

• the Arc you locked is mutably borrowed
• there was no other Arc because lock_strong_count required the strong count to be 1, meaning the Arc that locked it was the only one
• Weaks can't be upgrade in the meantime because the strong count being 0 will make them fail

@Aaron1011 for this same reason Drop is still sound, because it will never run while an Arc is locked

[SoniEx2]

We don't return a guard. We return a bool.

[SkiFire13]

Oh, I didn't see the methods were public. I was expecting them to be encapsulated in a public API like the one shown in the first post.

If this is the case then Drop is indeed leaky

[chorman0773]

The guards aren't necessarily useful, since you still can't do anything with the guard (safely) you couldn't already do with the Arc (due to variance). However, these primitives would be useful to writing higher level guard types that, through a wrapper such as Static (or with a T: 'static bound) could provide safe, statically checked, mutable access to an Arc.

[SkiFire13]

The guards aren't necessarily useful, since you still can't do anything with the guard (safely) you couldn't already do with the Arc (due to variance).

The fundamental idea behind this being able to provide an API akin to Arc::get_mut but that doesn't fail when Weaks still exists, instead temporarely prevents them from up upgrading. This is not possible with the current Arc.

[SoniEx2]

This check must be here no matter what, because, assuming a hypothetical borrow_mut that returns a guard, without this, you could cause UB in safe code by leaking the guard.

(Granted, the with_mut variant doesn't have this issue.)

[SoniEx2]

Anyway how do you benchmark changes to alloc/Arc...?

[the8472]

Currently there don't seem to be any benchmarks for Arc. You can write new ones and add them under library/alloc/benches and then run

x.py bench library/alloc --stage 0 --test-args <pattern for bench names>

[Mark-Simulacrum]

I am not personally convinced these merit being in std; there's a whole host of things you could do with the counters in an Arc, but if we're to start exposing functions like this I'm not convinced it's better than giving unsafe access to the counters directly (e.g., by exposing ArcInner).

r? @m-ou-se for T-libs-api consideration (potentially to say 'no')

[SoniEx2]

(turned out qcell is a lot more suitable for what we were doing.)