Ergonomics issue: RpcReplyPort with custom BytesConvertable impls

[blakehawkins]

Hi again!

I have implemented BytesConvertable for a custom type, something like this:

impl BytesConvertable for Value { ... }

This works well and fine on its own, but I wanted to express an RpcReplyPort in terms of a wrapper around Value, i.e. Option<Value> or Vec<Value>

Unfortunately, there's no blanket impl <T> BytesConvertable for Vec<T> where T: BytesConvertable, so this doesn't work.

Further, as a user, I can't impl BytesConvertable for Vec<Value> because it's not permitted to implement foreign traits for foreign types

I had a look at your existing BytesConvertable impls and noticed that you manually implement a family of serdes using macros.

I did also try to build a blanket impl myself but it got very messy when I ran into a lack of numeric traits

For reference, I was trying to wrangle some code like this:

impl<T> BytesConvertable for Vec<T> where T: BytesConvertable + Sized {
    fn into_bytes(self) -> Vec<u8> {
        let mut result = vec![0u8; self.len() * std::mem::size_of::<T>()];
        for (offset, item) in self.into_iter().enumerate() {
            result[offset * std::mem::size_of::<T>()
                ..offset * std::mem::size_of::<T>() + std::mem::size_of::<T>()]
                .copy_from_slice(&item.to_be_bytes());
        }
        result
    }
    fn from_bytes(bytes: Vec<u8>) -> Self {
        let num_el = bytes.len() / std::mem::size_of::<T>();
        let mut result = vec![T::MIN; num_el];

        let mut data = [0u8; std::mem::size_of::<T>()];
        for offset in 0..num_el {
            data.copy_from_slice(
                &bytes[offset * std::mem::size_of::<T>()
                    ..offset * std::mem::size_of::<T>() + std::mem::size_of::<T>()],
            );
            result[offset] = <T>::from_be_bytes(data);
        }

        result
    }
}

(and some other variations, e.g. trying to use const generics in place of std::mem::size_of::<T>(), etc)

I came across this PR that I think would add the special sauce to do an appropriate blanket impl for numerics and therefore I think also blanket impl for Vec<T> where T: BytesConvertable.

In summary:

• At the moment, the best a user can do if they want to RpcReplyPort<Collection<T>> seems to be to wrap Vec with a custom struct and then impl BytesConvertable for MyVecWrapper<Value>
• It would be nice to have a built-in blanket impl BytesConvertable for Vec<T> where T: BytesConvertable
• I'm wondering if you're aware/tracking this and/or have other suggestions
• More generally, BytesConvertable feels clumsy compared to using serde -- it would be nice to have serde integration, perhaps behind a feature flag?

[slawlor]

So in general I agree, and there's probably some blanket implementations I missed in BytesConvertable when I wrote it. The reasons I did it that way however are the following

1. I didn't want to force a dependency on serde everywhere, as often there's multiple "right ways" to do serialization.
2. I just needed a trait which did a basic to_bytes and from_bytes methods and really nothing else
3. I figured the downstream user could do what I did for protobuf, which is to wrap the implementation in a macro to automate the necessary implementation points.

I agree there's probably bits missing here, but I'm not yet confident I found a good way to handle it uniformly. That being said, let me see if I can come up with something for the Vec<_> case which I agree would be helpful without having to wrap it into a parent struct.

[slawlor]

I'm happily open to suggestions here, especially as it doesn't really impact the core ractor crate's APIs but the lesser-used ractor_cluster (so far lol). So if you, or anyone reading the issue, has suggestions I'd be happy to hear them. I expect ractor_cluster to have some churn in patterns as usage grows and features stabilize.

[blakehawkins]

Thanks for the swift reply!

So if you, or anyone reading the issue, has suggestions I'd be happy to hear them.

One option I can think of would be to write a blanket impl as impl <T> BytesConvertable for Vec<T> where T: ?Float + ?PrimInt using https://docs.rs/num-traits/latest/num_traits/

The idea would be to "avoid" the other Vec impls that you've added for Vec over numerics

i'd be happy to try something out if this is a direction you'd consider going in (assuming it works)

[slawlor]

i'd be happy to try something out if this is a direction you'd consider going in (assuming it works)

Please feel free to put something together! Would be happy to iterate on it, but really open to anything here as ractor_cluster is probably the most volatile part of the crates, so it's kind of expected to mutate at the same pace

[blakehawkins]

I've spent some time playing with this, and managed to do a few things here:

• replace the macro implementations of BytesConvertable on numerics using a blanket impl instead, which looks something like this (I threw away the working code):

impl<T> BytesConvertable for Vec<T>
    where T: ToBytes<Bytes = <T as FromBytes>::Bytes> + FromBytes + Default + Sized + Clone, <T as FromBytes>::Bytes: Sized
{
   fn into_bytes(self) -> Vec<u8> {
       self.into_iter().flat_map(|val| val.to_be_bytes().as_ref().to_owned().into_iter()).collect()
    }

   fn from_bytes(bytes: Vec<u8>) -> Self {
       let size: usize = <T as Default>::default().into_bytes().len();
       let num_el: usize = bytes.len() / size;
       let mut result = vec![<T as Default>::default(); num_el];

       let mut data = T::to_be_bytes(&<T as Default>::default());
       for offset in 0..num_el {
           let offs: usize = offset * size;
           data.as_mut().copy_from_slice(&bytes[offs..offs + size]);
           result[offset] = T::from_be_bytes(&data);
       }

       result
   }
}

• also wrote a blanket impl for "other" types which would work for non-numerics, using something like this (also threw away the code):

impl<T> BytesConvertable for Vec<T>
    where T: BytesConvertable + Default + Sized + Clone + !PrimInt
{
...

^ there were two problems with this that I failed to anticipate -- the first is that rust is limited on blanket impls -- you can't have two blanket impls for the same trait, even if there's a hard prevention of overlap (in fact you can't have blanket impls on !PrimInt at all).

The second problem is that even if this did work, you'd still need a way to deserialise values of unknown size.

So finally I wrote a working implementation by introducing a new "NullTerminated" trait.

Maybe you can take a look here and let me know if you think this would be a useful contribution?

The one other thing I would like to add is a custom derive macro for NullTerminated so that users can "opt-in", so the API would finally be something like this:

use ractor::BytesConvertable;

#[derive(NullTerminatedBytesConvertible)]
struct MyCustomValue {}

impl BytesConvertable for MyCustomValue {
  ...
}

#[test]
fn vec_of_custom_value() {
  let values = vec![MyCustomValue {}, MyCustomValue{}]
  
  let bytes = values.into_bytes();

  let result: Vec<MyCustomValue> = BytesConvertable::from_bytes(bytes);

  // assert values == result
}

There are some problems with using null-terminator, i.e. if the serialised version of your type has actual null bytes in it then they'll get incorrectly treated as deliminators during the from_bytes step. One improvement might be to use const generics, replacing NullTerminatedBytesConvertible with TerminatedBytesConvertible<{ const T: u8 }> or so

[slawlor]

Thanks for the follow-up, I'll take a look when I get a sec!

[slawlor]

OK I finally had a chance to look into this, and this is a really great exploration of the problem so first off thanks for that!

Just a thought, since we kind of do it implicitly in some BytesConvertable implementations, what about instead of a terminated byte vector, what about a length prepended byte array? That way you don't have to worry about special characters, and decoding just needs to append the length at the start? Might be a safer design, just an idea.

[slawlor]

And I'm also not set-in-stone on BytesConvertible, maybe it would make sense to include length prepending into trait directly