Skip to content
This repository has been archived by the owner on May 6, 2018. It is now read-only.

length_delimited should allow using a custom Codec #63

Closed
oberien opened this issue Aug 9, 2017 · 4 comments
Closed

length_delimited should allow using a custom Codec #63

oberien opened this issue Aug 9, 2017 · 4 comments

Comments

@oberien
Copy link

oberien commented Aug 9, 2017

I can't find a way to use a custom Codec in conjuction with length_delimited. I'm currently working on a PR to implement varint support for length_delimeted (an untested work in progress can be found here). The usecase is to use protobuf with a varint length delimiter. With length_delimited::Framed only implementing Stream and Sink, but not AsyncRead and AsyncWrite, it's not possible to chain a Codec with AsyncRead::framed on it in order to deserialize the returned BytesMut / serialize items into BytesMut before length-delimiting them.

My suggestion would be to either implement AsyncRead and AsyncWrite on Framed, and respectively on FramedRead and FramedWrite.
Another idea is to be able to add a custom Codec to Builder, which will be invoked after a full frame has been read into BytesMut for deserialization, and for serialization in order to create the BytesMut which needs to be length-delimited.

@carllerche
Copy link
Member

Here is an example: https://github.com/carllerche/tokio-serde-json/blob/master/src/lib.rs

The strategy is to "map" the Stream<Item=Bytes> -> Stream<Item=MyTime. Basically you can use regular stream combinators instead of something custom.

@oberien
Copy link
Author

oberien commented Aug 10, 2017

I think that this is a lot of overhead and work to write for every possible serialization format possible. Would it be possible to offer this in a more generic way? For the read part it's easy enough to use and_then:

let length_delimited = Builder::new()
    .varint()
    .new_framed(stream);
let (write, read) = length_delimited.split();
let read = read.and_then(decode);

fn decode(buf: BytesMut) -> io::Result<Item> { ... }

But for the write part it's rather hard to wrap encoding around the sink. Instead, I need to wrap it around a stream which produces items / manually call encode / write bloaty code like it's done in tokio-serde-json.

I think that from a user experience point of view the idea of tokio_io::codec::Framed is pretty nice. You just need to write a codec and automatically have the whole stream wrapped. Using this kind of approach in conjunction with length_delimited would be even better because you don't need to take care about manually length-prefixing the message and to verify the correct length of a frame. Therefore, I think it would be a nice user interface to be able to provide an implementation of a Codec to length_delimited which both the stream and sink will automatically get wrapped with just like it's done with tokio_io::codec::Framed.

EDIT: I just found the Sink::with method, which is what I tried to accomplish. So I guess using Stream::and_then and Sink::with is fair enough. I do think that there should be an example for this, though.
Is there a particular reason why you wrote tokio-serde-json with completely new types if you could have just used those combinators instead?

@carllerche
Copy link
Member

You probably wouldn't split, and you can use Sink::with to do the mapping.

It would be easier if map and with took types vs. functions... That said, if you want to explore this idea further, you can do so in a standalone git repo like I did w/ tokio-serde / tokio-serde-json. Once it gets fleshed out, we can consider if there is anything worth merging into tokio-io.

@oberien
Copy link
Author

oberien commented Aug 11, 2017

Given Sink::with and Stream::map / Stream::and_then I guess this issue is resolved. The idea of taking types instead of functions sonuds very interesting and could even be implemented in a non-breaking way (implementing the type for functions). Unfortunately I don't have the time currently to dig into that rabbit hole.
Anyway, thanks for your help.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants