atomic_refcell icon indicating copy to clipboard operation
atomic_refcell copied to clipboard

Add map-like methods to AtomicRef(Mut) which work with new objects rather than references

Open LechintanTudor opened this issue 4 years ago • 7 comments

Having map methods for reference types which can only work with sub-borrows of the original object is a bit too restrictive in my opinion, and it doesn't allow certain patterns as the one presented bellow. My solution is to add an additional method to all reference types, map_into, which can create entirely new objects with the same lifetime as the original. This change requires adding additional reference types that hold T's, instead of &(mut) T's, which I've called OwnedAtomicRef and OwnedAtomicRefMut.

Here is the problem that adding map_into would solve

let values: (Box<dyn Any>, Box<dyn Any>) = (Box::new(1_u32), Box::new(2_u32));
    let cell = AtomicRefCell::new(values);

    // Create an OwnedAtomicRefMut to hold our new object which borrows the original
    let mut borrowed_values = AtomicRefMut::map_into(cell.borrow_mut(), |values| {
        (
            Box::as_mut(&mut values.0).downcast_mut::<u32>().unwrap(),
            Box::as_mut(&mut values.1).downcast_mut::<u32>().unwrap(),
        )
    });
    
    // Set the values while still holding a lock
    *borrowed_values.0 = 100;
    *borrowed_values.1 = 200;

LechintanTudor avatar Feb 16 '21 18:02 LechintanTudor

I successfully implemented all the functionality described above. Please let me know if you are interested in a PR.

LechintanTudor avatar Feb 16 '21 19:02 LechintanTudor

Thanks, and sorry for the lag. So the basic issue here is that you want to use a tuple of borrows, but the tuple is technically not a borrowed type, right?

My intuition is that the duplication across {,Owned}AtomicRef{,Mut} would make the code more complex. So unless we find an elegant way to generalize across the permutations, I'm hesitant to take such a patch absent strong demand from multiple consumers. However, I'll leave this issue open so that others can comment if they run into the same issue.

bholley avatar Mar 25 '21 16:03 bholley

So the basic issue here is that you want to use a tuple of borrows, but the tuple is technically not a borrowed type, right?

Yes that's exactly it.

LechintanTudor avatar Mar 25 '21 19:03 LechintanTudor

I have a similar need.

I have a type such as AtomicRef<UntypedStorage>, and there's a method to convert a borrow of &'a UntypedStorage to a TypedStorage<'a, T>, but I couldn't do that in AtomicRef.map because TypedStorage is actually an owned type, with the same lifetime as the &UntypedStorage reference, not a reference to a field of UntypedStorage.

zicklag avatar Dec 16 '22 20:12 zicklag

For anybody else who needs it, I was able to accomplish what I needed using the atomicell crate. It has an unsafe method RefMut::into_split() that allows you to get access to the borrow guard and the reference separately, at which point, you must ensure that the reference is never accessed after the borrow guard is dropped, but you can create your own MappedRefMut or whatever you need for your use-case using the guard and the reference.

zicklag avatar Aug 23 '23 13:08 zicklag

at which point, you must ensure that the reference is never accessed after the borrow guard is dropped, but you can create your own MappedRefMut or whatever you need for your use-case using the guard and the reference.

Note that there is some subtle behavior that you need to be aware of to do this soundly. References passed as function parameters (either directly or included in a type) are treated as if they could be used up until the end of that function even if they aren't used after a certain point (or to put it another way they are expected to not be aliased incorrectly for the entirety of the function).

For an example of how this manifests see https://github.com/bholley/atomic_refcell/pull/18

To disable this you can wrap the reference/type containing the reference in MaybeDangling from https://docs.rs/maybe-dangling.

For details on why this exists: https://perso.crans.org/vanille/treebor/protectors.html.

Imberflur avatar Aug 23 '23 19:08 Imberflur

Big thanks for pointing that out to me! That's a very easy foot-gun to miss. I'll look into a PR for atomicell.

zicklag avatar Aug 23 '23 23:08 zicklag