tokio-rs / bytes

Utilities for working with bytes
MIT License
1.91k stars 286 forks source link

add `Bytes::is_unique` #643

Closed Cyborus04 closed 9 months ago

Cyborus04 commented 10 months ago

Closes #533

This adds the is_unique method to Bytes. It checks reference count when backed by (effectively) Arc<Vec<u8>>, returns true when backed by a vec, and returns false for static slices

xonatius commented 9 months ago

I think there might be an issue with this api: if is_unique returns true, there is no guarantee that it is still unique immediately after this call, as some other thread can hold the reference to this Bytes object and clone it while is_unique check is performed.

Cyborus04 commented 9 months ago

Ah, like if it was an Arc<Bytes>. Strange case, but still an issue. I think changing it to take &mut self should solve that?

xonatius commented 9 months ago

I was thinking about &Bytes rather than Arc<Bytes> (Bytes is Sync, right?), but yes - taking &mut self in this method should ensure this never happens (:

Darksonn commented 9 months ago

Normally you would add a method to the vtable and call it, rather than compare with the different vtables. That makes it more difficult to add more vtables in the future.

Cyborus04 commented 9 months ago

Makes sense. One moment, then.

Darksonn commented 9 months ago

You can add a comment that mentions that this could happen if the Bytes is used concurrently, but other than that, I don't think the concern that @xonatius mentioned is a problem. You shouldn't use &mut self just for that reason.

Cyborus04 commented 9 months ago

I agreed with the &mut self change since I have safety on the mind, as I was making this change hoping to later also add a method to get mutable access if the Bytes is unique. Even then, try_get_mut (or any other name) could itself take &mut self instead of this needing to, so I'll still revert it.

Cyborus04 commented 9 months ago

I have detailed my motivation for this in hyperium/http#662