Documentation of Arc::from_raw is unnecessarily restrictive

[terrarier2111]

Location

Arc::from_raw

Summary

Arc::from_raw mentions that the provided pointer has to have been returned from Arc::into_raw but it doesn't mention Arc::as_ptr at all. This disallows many usages that depend on not modifying the reference counter while passing Arc around in form of a pointer and converting it back by calling Arc::from_raw on the pointer and wrapping the result inside a ManuallyDrop (as after one conversion round of Arc::into_raw -> ManuallyDrop<Arc::from_raw> -> Arc::as_ptr would disallow recreating the Arc through Arc::from_raw)

[saethlin]

Isn't this code pattern you want documented as valid unnecessarily dangerous, like Vec::from_raw_parts with a pointer from Vec::as_ptr?

Why do you want to pass around an Arc created like this instead of a reference or a raw pointer?

[albertlarsan68]

@rustbot claim

[lukaslueg]

Beware of the footgun: To call Arc::into_raw, you consume one reference count, as into_raw consumes the Arc; this is the symmetry with from_raw. On the other hand, as_raw takes an &Arc, so its possible to call this many times wrt the same reference count; if you then call from_raw on those pointers, the reference count will underflow when those newly created Arcs get dropped.
Obviously we are in unsafe-land anyway, but the docs should point this out explicitly, as as_raw+from_raw is much easier to get wrong than into_raw/from_raw

[terrarier2111]

Isn't this code pattern you want documented as valid unnecessarily dangerous, like Vec::from_raw_parts with a pointer from Vec::as_ptr?

Why do you want to pass around an Arc created like this instead of a reference or a raw pointer?

I was able to find a different solution using into_raw and from_raw, but i still think mentioning the possibility of using Arc::as_raw should be mentioned.

[terrarier2111]

Beware of the footgun: To call Arc::into_raw, you consume one reference count, as into_raw consumes the Arc; this is the symmetry with from_raw. On the other hand, as_raw takes an &Arc, so its possible to call this many times wrt the same reference count; if you then call from_raw on those pointers, the reference count will underflow when those newly created Arcs get dropped. Obviously we are in unsafe-land anyway, but the docs should point this out explicitly, as as_raw+from_raw is much easier to get wrong than into_raw/from_raw

Yea, you are absolutely right with that but by passing around ManuallyDrop<Arc> this can be largely avoided (except if one manually drops the Arc)

[pitaj]

Why would you need to do ManuallyDrop::new(from_raw(as_ptr)) instead of frow_raw(into_raw(clone))?

I can't imagine avoiding a single atomic increment brings measurable benefit.

[terrarier2111]

Why would you need to do ManuallyDrop::new(from_raw(as_ptr)) instead of frow_raw(into_raw(clone))?

I can't imagine avoiding a single atomic increment brings measurable benefit.

If you want to you can perform measurements, this is the repo: https://github.com/terrarier2111/SwapArc
(it's essentially a data structure that presents a faster alternative to ArcSwap)
but yea generally speaking on the fast path i only have a single Relaxed load and even on the slow paths i try to use as few cmp_exchg and fetch_xxx operations as possible because these are pretty much the most expensive atomic operations i am using, even without contention (at least on x86_64), also every atomic modification of the counter invalidates its cache line which can also be very punishing. But i am not alone with this concern, the author of ArcSwap also noticed the impact of Arc cloning on performance in his implementation.

[pitaj]

Could you use something like Arc::as_weak instead? #100472

[terrarier2111]

I can't because that would mean returning a type that's not just T (in Arc<T>) which is a requirement for my API (as it is generic and allows to use more than just Arc)

[pitaj]

Couldn't you put an associated type on DataPtrConvert? For instance:

impl<T: Send + Sync> DataPtrConvert<T> for Arc<T> {
    type Weak = sync::sync::Weak<T>;

    fn as_weak(&self) -> Self::Weak { todo!() }
}

[terrarier2111]

Couldn't you put an associated type on DataPtrConvert? For instance:

impl<T: Send + Sync> DataPtrConvert<T> for Arc<T> {
    type Weak = sync::sync::Weak<T>;

    fn as_weak(&self) -> Self::Weak { todo!() }
}

Uhm maybe that could work, i am not really experienced in working with associated types.
Does Weak introduce any overhead for example branches or the like to check whether the "weak link" is still valid, or doesn't it check that and it's free?

[ultimaweapon]

I also having the exact use case of this. I'm in the process of migrating the C++ application to Rust and what I did is building a Rust crate as a static library and do something like this:

#[no_mangle]
pub unsafe extern "C" fn run(
    argc: c_int,
    argv: *mut *mut c_char,
    cpp: unsafe extern "C" fn(*const App, c_int, *mut *mut c_char) -> c_int
) -> c_int {
    let app = Arc::new(App {});
    let code = cpp(Arc::as_ptr(&app), argc, argv);

    // Graceful shutdown code here.
    code
}

The run function will be called immediately when the execution landed at the C++ entry point. When the control is transferred back to C++ side it will have a pointer to App that returned from Arc::as_ptr(&app), which can be passed back to some Rust functions. The problem is some Rust functions required to spawn a background task that required to clone the App object into it, which currently cannot be done in an elegance way.

[ultimaweapon]

Seems like what I actually need is something like clone_from_raw instead.

[Dylan-DPC]

Closing this as this isn't bug or there's anything to be done here

[ultimaweapon]

I think what needs to be done is update from_raw documentation to allow a pointer from as_ptr.