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.

Sign up for GitHub

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

Jump to bottom

Buffer writes before chunks are written to connection #72

Merged
merged 16 commits into from
May 21, 2024
Merged

Buffer writes before chunks are written to connection #72

merged 16 commits into from
May 21, 2024

Conversation

rmja
Copy link
Member

@rmja rmja commented May 17, 2024

This commit:

  • Moves the responsibility for writing the request body from Request to HttpConnection.
  • Uses the buffer provided when calling into_buffered() to buffer writes before they are passed on to the ChunkedBufferWriter

This fixes #71

@rmja
Copy link
Member Author

rmja commented May 17, 2024

cc @lulf @bugadani

@rmja rmja marked this pull request as draft May 17, 2024 15:44
@rmja
Copy link
Member Author

rmja commented May 17, 2024

I could use some help making this better.
The problem is now that there are multiple connection flushes during the writing of a request which is quite annoying.

I feel like it somehow should be possible to buffer internally in in the chunked writer in a way such that the chunks gets as large as possible up to the max buffer size.

Any help is greatly appreciated.

@rmja
Copy link
Member Author

rmja commented May 17, 2024

I have now added a BufferedChunkedBodyWriter which now has the responsibility of both buffer and chunk. It will probably need some tests.

@rmja rmja marked this pull request as ready for review May 21, 2024 07:05
@rmja
Copy link
Member Author

rmja commented May 21, 2024

This is now ready for review. Let me know what you think.

bugadani
bugadani reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

Looks mostly OK to me, although I wasn't particularly thorough.

src/body_writer.rs Outdated Show resolved Hide resolved
src/body_writer.rs Outdated Show resolved Hide resolved
src/body_writer.rs Show resolved Hide resolved
bugadani
bugadani approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

Thank you, I especially appreciate the myriad of added tests!

rmja added 16 commits May 21, 2024 12:06
@rmja @bugadani
Buffer writes before chunks are written to connection
85c20cd 
This commit:
* Moves the responsibility for writing the request body from `Request` to `HttpConnection`.
* Uses the buffer provided when calling `into_buffered()` to buffer writes before they are passed on to the `ChunkedBufferWriter`

This fixes drogue-iot#71
@rmja @bugadani
This requires buffered-io 0.5.1
609cf4b
@rmja @bugadani
Fix clippy warning
b66eb13
@rmja @bugadani
Fix compilation without tls
ab074c6
@rmja @bugadani
Change ChunkedWriterStrategy to minimize connection writes
f80172a
@rmja @bugadani
Fix rename
9d76971
@rmja @bugadani
Revert "Change ChunkedWriterStrategy to minimize connection writes"
c2d40f9 
This reverts commit 87353be.
@rmja @bugadani
Revert "Fix rename"
c6d1f59 
This reverts commit 5b28015.
@rmja @bugadani
Add BufferedChunkedBodyWriter
cbae22c
@rmja @bugadani
Move body writers to separate file
2df80ca
@rmja @bugadani
Add tests and optimize allocated header to match the remaining size o…
5949cb8 
…f the buffer
@rmja @bugadani
Fix test for any buffered chunk bytes in flush()
564b5c4
@rmja @bugadani
Rename emit_finished_chunk to emit_finished_chunks
6a58d14
@rmja @bugadani
Ensure that we clear the BufferedWrite that we inherit
ffdf480
@rmja @bugadani
Fixes according to review - thanks @bugadani !
3c6cd6f
@rmja @bugadani
Fix write_chunked_header to support zero chunk length
a813563
@bugadani bugadani merged commit c9762fe into drogue-iot:main May 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bugadani bugadani bugadani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Undesired small chunks when writing buffered chunked body
2 participants
@rmja @bugadani