-
Notifications
You must be signed in to change notification settings - Fork 118
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
don't flush on every body part #214
base: main
Are you sure you want to change the base?
don't flush on every body part #214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this achieves quite what we want
@@ -767,9 +767,10 @@ extension TaskHandler: ChannelDuplexHandler { | |||
|
|||
return body.stream(HTTPClient.Body.StreamWriter { part in | |||
context.eventLoop.assertInEventLoop() | |||
return context.writeAndFlush(self.wrapOutboundOut(.body(part))).map { | |||
context.write(self.wrapOutboundOut(.body(part))).whenSuccess { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, this would now never flush any body which means we'll load the whole body into memory before sending.
I think the right thing to do is:
- if you know that whole body straight away (without streaming), then we should do
write(.head)
,.write(.body)
,.write(end)
,flush
- if we actually want to stream the body, then
writeAndFlush(.head)
,writeAndFlush(.body)
,writeAndFlush(.end)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need a test actually that shows that this PR doesn't work. We should have a test which checks that body chunks do arrive at the other end before the next bit is sent out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a heads up, the main development branch has been changed to This PR has been re-targeted to main and should just work. However when performing rebases etc please keep this in mind -- you may want to fetch the main branch and rebase onto the |
Right now we call
flush
on every body part write, this can be suboptimal, closes #203Motivation:
Library users can write data in small chunk, in this case we are not buffering enough data, better solution would be to buffer at the NIO level in this case.
Modifications:
Body part write is not not flushing, just
write
Result:
Body part writes are not flushing anymore