- Notifications
You must be signed in to change notification settings - Fork136
don't flush on every body part#214
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
weissi left a comment
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
| return body.stream(HTTPClient.Body.StreamWriter{ partin | ||
| context.eventLoop.assertInEventLoop() | ||
| returncontext.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 nownever 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.
ktoso commentedAug 20, 2020
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
flushon 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
writeResult:
Body part writes are not flushing anymore