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

Expose RecvStream::poll_read & poll_read_chunk #1784

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

jean-airoldie
Copy link
Contributor

Closes #1782

@jean-airoldie
Copy link
Contributor Author

Now for this one, the poll_read_chunk is not part of AsyncRead, so technically this would force you to support that new method, but I figured that if it might result in improved read performance, that might be desirable to expose too.

@jean-airoldie
Copy link
Contributor Author

In my use case, I'm thinking of using poll_read to read the size prefix, followed by poll_read_chunk to read the actual message, for instance.

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured that if it might result in improved read performance, that might be desirable to expose too.

This is speculative. I'd prefer to avoid further complicating our API without a concrete use case that benefits, particularly as using poll_read_buf correctly is itself complex. For example, your use case would need to handle consuming any number of chunks before reaching the desired total length, and you'd then have to either deal with having non-contiguous data in whatever downstream processing you do, or copy everything into a single buffer, at best duplicating the logic already provided by poll_read.

quinn/src/recv_stream.rs Outdated Show resolved Hide resolved
@jean-airoldie
Copy link
Contributor Author

jean-airoldie commented Mar 17, 2024

This is speculative. I'd prefer to avoid further complicating our API without a concrete use case that benefits, particularly as using poll_read_buf correctly is itself complex.

Fair enough. In my understanding, the potential benefit was from the unordered read, even if you have to copy the chunks into a separate buffer, in the same way the read_to_end method is implemented.

@djc
Copy link
Member

djc commented Mar 18, 2024

(Needs a rebase. Would be nice if you can squash the commits here.)

@djc djc merged commit a5b6fac into quinn-rs:main Mar 19, 2024
8 checks passed
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.

Expose poll_write & poll_close
3 participants