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

Implement BufQueue type #371

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

stepancheg
Copy link
Contributor

BufQueue<B: Buf> is a VecDeque<B> which implements Buf and
caches the remaining size.

This utility can be used to implement outgoing network buffer.
Imagine this use case:

type Header = Cursor<[u8; 5]>;
type Payload = Bytes;
type MyMessage = Chain<Header, Payload>;

outgoing_queue: BufDeque<MyMessage>;

New messages can be queues by simply appending to this BufQueue,
and the queue can be flushed by invoking bytes_vectored on the
queue object.

And both enqueue and flush operations are zero-copy.

remaining cache is needed to optimize bytes operation, which
might be expensive if the queue is long.

I'm not 100% sure this is how Bytes is meant to be used, but this
BufQueue type seems to be the most natural way.

`BufQueue<B: Buf>` is a `VecDeque<B>` which implements `Buf` and
caches the remaining size.

This utility can be used to implement outgoing network buffer.
Imagine this use case:

```
type Header = Cursor<[u8; 5]>;
type Payload = Bytes;
type MyMessage = Chain<Header, Payload>;

outgoing_queue: BufDeque<MyMessage>;
```

New messages can be queues by simply appending to this `BufQueue`,
and the queue can be flushed by invoking `bytes_vectored` on the
queue object.

And both enqueue and flush operations are zero-copy.

`remaining` cache is needed to optimize `bytes` operation, which
might be expensive if the queue is long.

I'm not 100% sure this is how `Bytes` is meant to be used, but this
`BufQueue` type seems to be the most natural way.
@Ralith
Copy link

Ralith commented Feb 11, 2020

This seems handy, but so long as bytes is an extremely stability-sensitive interface crate I don't know if it should be accepting new self-contained interfaces. Perhaps this could be a separate crate?

@stepancheg
Copy link
Contributor Author

Perhaps this could be a separate crate?

Or users can simply implement this themselves, BufQueue is not rocket science.

It took me a very long time to figure out how to connect Bytes, Buf and tokio IO interfaces for the described use case. I guess it might be the case for other people too, and some default implementation might be useful for that, at least for illustration.

@seanmonstar
Copy link
Member

This seems handy, but so long as bytes is an extremely stability-sensitive interface crate I don't know if it should be accepting new self-contained interfaces.

This has been the stance so far, and is why hyper has the type inlined (and not exposed directly). We've discussed that something similar might make sense like in tokio-codec or tokio-utils or something. But as you've also mentioned, it's not difficult for someone to just have in their own project, it's incredibly small.

@Duckilicious
Copy link

I stumbled across this while looking for the standard solution tokio provides for cases where you want to look at a few payloads as one continuous payload and I am bit surprised you don't think this deserves a place in the provided interfaces but something like pub struct Chain<T, U> does. Even if there is somehow a way to use Chain primitive in the same way you use this the remaining() op is still too costly. I think merging this PR would be beneficial.

I feel like the provided API isn't complete without the suggested primitive and I'd like you to reconsider your stance regarding your decision to drop it. Moving the burden for every project that requires something like this and uses tokio to maintain it's own version doesn't seem like a very user friendly approach.

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

Successfully merging this pull request may close these issues.

4 participants