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

Add SeekBuf trait and BufCursor implementation #658

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

dylanplecki
Copy link

@dylanplecki dylanplecki commented Jan 19, 2024

Fixes #657.

This PR introduces the SeekBuf trait for buffers that support arbitrary lookups of memory, and a new BufCursor struct for immutably iterating over a SeekBuf type.

SeekBuf is strictly an opt-in trait with additional functionality over the standard Buf trait and otherwise does not change the API of this crate.

The BufCursor type provides compatibility to a wide range of core/std Rust features through its Iterator implementation. It also allows recursive and non-consuming access to the full memory of a buffer that was otherwise not possible via the Buf trait.

src/buf/vec_deque.rs Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor

I'm really sorry that I didn't get around to responding to this when you posted your other feature requests.

It's pretty normal to do something like this by taking a &[u8] into the buffer and using the normal Buf operations on the &[u8]. What does this add that we can't already do with that? I guess non-contiguous buffer support?

You're adding a lot of complex code using indexes in your BufCursor. This makes me rather wary. Index math is error-prone, and Rust's safety guarantees doesn't really prevent mistakes. The bytes crate is only passively maintained these days; there's not much active development. I'm reluctant to add anything that is likely to introduce bugs.

@dylanplecki
Copy link
Author

@Darksonn no worries! Thanks for the feedback.

This change intends to provide additional functionality for library code that accepts a Buf impl, rather than a library that creates one or just uses it internally. It's very useful for code that can backtrack in the buffer during parsing.

For example, let's say that I have a method that accepts something along the lines of an impl Buf, and the method needs to do something like parse an HTTP/1 header or read some struct bytes of fixed length/depth. The only reasonable way to then parse that buf is to Clone it each time we want to read bytes, if we have the possibility of backtracking (failed parse).

As the library maintainer, we have no guarantee how cheap the Clone operation is on the buf that was passed in. It may be cloning a slice pointer/len, or cloning an entire heap-allocated Vec. So we lose any performance guarantees we may have had with the typing system once we accept arbitrary Buf types.

Since buffers may also be non-contiguous, adding bounds like AsRef<[u8]> for cheap cloning may not satisfy all implementations of Buf (like VecDeque).

Outside of impl Buf uses, this also provides full immutable access to the underlying memory on &dyn Buf types that will likely not have a compatible Clone implementation (this was actually my primary motivating factor in adding this type). This is used in a buffer filter chain where filters pass the dyn Buf between each other and may overwrite it with their own implementation.

I definitely understand that this library is primarily used by Tokio and is provided as a utility with no guarantees that anything beyond the scope of Tokio will be implemented. I also agree that the implementation is quite complex beyond the guarantees of the Rust compiler (this is hopefully well-covered by the added unit tests), so will add a maintenance burden to the project.

For my use case, I've settled on maintaining a small fork of this library with the additional functionality that may not be useful for Tokio or other core use-cases of the bytes crate.

If you think that there isn't much utility in adding this functionality to this crate, or would prefer not to maintain this code long-term, I would be happy to close this as "won't fix" and retain the scope of my usage to my fork.

Thanks again for the review!

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.

Feature Request: Seekable Buffer ("SeekBuf") and cursor/iterator support
2 participants