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 BufRead for BodyReader #45

Merged
merged 18 commits into from
Oct 23, 2023
Merged

Implement BufRead for BodyReader #45

merged 18 commits into from
Oct 23, 2023

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 18, 2023

This PR allows users to read from a TLS connection without copying. Non-TLS sessions will be able to save some memory by reusing the initial read buffer that was passed in to store the HTTP headers.

@bugadani bugadani marked this pull request as ready for review October 18, 2023 11:55
@bugadani

This comment was marked as outdated.

@bugadani bugadani marked this pull request as draft October 18, 2023 12:01
@bugadani bugadani force-pushed the buffer branch 4 times, most recently from 208610d to 95ade8a Compare October 18, 2023 12:55
@bugadani bugadani marked this pull request as ready for review October 18, 2023 12:55
@bugadani

This comment was marked as outdated.

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.

I haven't used these new APIs that much so I'll let @rmja comment as well, but I think it looks good.

@bugadani
Copy link
Contributor Author

I haven't used these new APIs

The question is more that this PR makes the old API slightly slower :)

@rmja
Copy link
Member

rmja commented Oct 20, 2023

Is it correctly understood that the underlying non-tls connection now must implement BufRead? Could this be more a conditional constraint such that the BodyReader implemented BufRead if the underlying connection did?

@bugadani

This comment was marked as resolved.

@bugadani bugadani marked this pull request as draft October 20, 2023 14:47
@bugadani bugadani marked this pull request as ready for review October 20, 2023 15:30
src/response.rs Outdated Show resolved Hide resolved
@bugadani bugadani marked this pull request as draft October 21, 2023 19:26
@bugadani bugadani marked this pull request as ready for review October 21, 2023 19:27
@lulf lulf merged commit 1890da8 into drogue-iot:main Oct 23, 2023
1 check passed
@bugadani bugadani deleted the buffer branch October 23, 2023 09:11
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.

3 participants