Ergonomics issue: RpcReplyPort with custom BytesConvertable impls about ractor HOT 8 OPEN

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.

from ractor.

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

from ractor.

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

from ractor.

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

from ractor.

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.

from ractor.

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)

from ractor.

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.

from ractor.

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

from ractor.