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

don't flush on every body part #214

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

artemredkin
Copy link
Collaborator

Right now we call flush on every body part write, this can be suboptimal, closes #203

Motivation:
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

@artemredkin artemredkin requested review from weissi and Lukasa May 18, 2020 17:14
Copy link
Contributor

@weissi weissi left a 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 {
Copy link
Contributor

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)

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

wrote #219 to test this. This PR should go in after #219 to prove that it works.

@ktoso ktoso changed the base branch from master to main August 20, 2020 01:30
@ktoso
Copy link
Contributor

ktoso commented Aug 20, 2020

Just as a heads up, the main development branch has been changed to main, following the Swift policy on this.

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 main branch from now on since master is not up-to-date anymore and is going to be deleted shortly.

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.

AsyncHTTPClient calls flush after writing each part of the HTTP message
3 participants