Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Producer::send can't send Vec<u8> or [u8] #60

Closed
leshow opened this issue Jan 29, 2020 · 4 comments
Closed

Producer::send can't send Vec<u8> or [u8] #60

leshow opened this issue Jan 29, 2020 · 4 comments

Comments

@leshow
Copy link
Contributor

leshow commented Jan 29, 2020

As far as I can tell, it's not possible to send binary data to a Producer.

send requires:

<T: SerializeMessage>(...message: &T)

There is a SerializeMessage impl for [u8], but none for Vec<u8>.

If you try to send a [u8], rustc will complain that the value is potentially unsized:

error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
   --> src/sinks/pulsar.rs:100:58
    |
100 |         let fut = self.producer.send(self.topic.clone(), &message[..]);
    |                                                          ^^^^^^^^^^^^ doesn't have a size known at compile-time

If you try to send a Vec<u8> or &Vec<u8>, it won't work because there is no SerializeMessage impl for it.

fix: Adding a bound like T: SerializeMessage + ?Sized is a quick fix to get [u8] data working. Vec<u8> should probably also be implemented. In the long run, it may be worth looking at the interplay between message: &T and serialize_message(input: &Self).

Depending on what you're looking for, I can make one of these changes.

@stearnsc
Copy link
Collaborator

I'm totally fine adding ?Sized to the signature as a fix to get it working now (that will also fix the str impl). Adding an impl for Vec<u8> also doesn't bother me, although I was thinking the slice impl would cover that use case.

Long term, I don't feel like I've fully thought through the implications of the Serialize/DeserializeMessage traits as the basis for handling this problem. Given the way the proto works, we need to have owned data in the payload, but most instance of serialization (thinking serde here) will involve copying data at some point, so for those, taking by ref seems to make the most sense. However, that makes it impossible to just pass in an owned Vec without falling back to send_raw.

Alternatively, we could change SerializeMessage::serialize_message to take Self by value, change the signature of send and send_all to take T: SerializeMessage by value, and then impl SerializeMessage for borrowed types in cases where it makes sense (&Foo instead of Foo). My immediate instinct is that that approach feels like an improvement, but I don't feel like I've thought it through enough.

@leshow
Copy link
Contributor Author

leshow commented Jan 29, 2020

I fiddled around with it earlier today when I originally came across this problem and made Self by value. It works fine, the only problem there is that you may only have a reference to your data, in which case you'll have to clone it at the call site.

I think there could be a third solution, maybe Message holds onto a reference and has a lifetime, like:

struct Message<'a> {
   payload: &'a [u8],
...
}

then,

impl<'a> SerializeMessage for &'a [u8] {
   fn serialize_message(payload: Self) -> Message {
      Message { payload, ..Default::default()
   }
}

I haven't actually tried this though, but something to think about. It could even be something like:

struct Message<'a, B> {  // I believe B: 'a is elided now since 2018
   payload: &'a B,
...
}

Then bound <B: AsRef<[u8]>> in it's impl's.

@leshow leshow mentioned this issue Jan 29, 2020
@stearnsc
Copy link
Collaborator

Unfortunately Message has to be 'static because it's immediately sent by channel to the async runtime to be handled.

the only problem there is that you may only have a reference to your data, in which case you'll have to clone it at the call site.

This works if you impl SerializeMessage for a borrow of your struct, something like

#[derive(Debug, Clone, Serialize)]
pub struct Foo {
    pub a: String,
    pub b: u64,
}

impl SerializeMessage for &Foo {
    fn serialize_message(input: &Foo) -> Result<producer::Message, Error> {
        let payload = serde_json::to_vec(input).map_err(..)?;
        Ok(producer::Message { payload, ..producer::Message::default() })
    }
}

then you should be able to still call producer.send(..) with your &Foo

@stearnsc
Copy link
Collaborator

stearnsc commented Feb 4, 2020

Fixed in #61 (at least until we make SerializeMessage work by value instead of by ref)

@stearnsc stearnsc closed this as completed Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants