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

Allow read_to_end with ChunkedEncoding #61

Merged
merged 11 commits into from
Nov 28, 2023
Merged

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Nov 11, 2023

I had to do a bunch of refactoring, because I had to create an abstraction for maybe-BufRead types (and I had to implement that for the tokio adapter stuff). This is because ChunkedEncoding's new reader function works via BufRead.

A downside of this PR is that read_to_end is only available for TryBufRead types, which we only define for HttpConnection in the library, and the tokio adapter in the tests.

The PR can most likely be simplified, I was just aiming to get it working and it took long enough.

if let HttpConnection::Tls(ref mut tls) = self.stream {
return tls.fill_buf().await.map_err(|e| e.kind());
// The matches/if let dance is to fix lifetime of the borrowed inner connection.
if self.stream.try_fill_buf().await.is_some() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rmja This got handy here, too :)

@bugadani
Copy link
Contributor Author

Do we want to return BufferTooSmall errors in the "outer" read_to_end?

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

Sorry I missed this. Lgtm!

@bugadani bugadani marked this pull request as draft November 21, 2023 07:57
@bugadani
Copy link
Contributor Author

Now this PR comes with a fix to an absolutely embarassingly dumb mistake :)

@bugadani bugadani marked this pull request as ready for review November 21, 2023 08:33
Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@bugadani bugadani merged commit ab10c14 into drogue-iot:main Nov 28, 2023
1 check passed
@bugadani bugadani deleted the end branch November 28, 2023 17:56
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.

2 participants