Read response body for TCP connection reuse #1576

itsksaurabh · 2020-07-16T14:05:40Z

According to the official Go docs:
https://golang.org/pkg/net/http/#Body

// The http Client and Transport guarantee that Body is always
// non-nil, even on responses without a body or responses with
// a zero-length body. It is the caller's responsibility to
// close Body. The default HTTP client's Transport may not
// reuse HTTP/1.x "keep-alive" TCP connections if the Body is
// not read to completion and closed.

To ensure http.Client connection reuse, I did the following things:

Read until Response is complete (i.e. ioutil.ReadAll(resp.Body) or similar)
Call Body.Close()

Closes #1575

googlebot · 2020-07-16T14:05:45Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

codecov · 2020-07-16T14:07:07Z

Codecov Report

Merging #1576 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1576      +/-   ##
==========================================
+ Coverage   67.95%   67.98%   +0.03%     
==========================================
  Files          96       96              
  Lines        8762     8772      +10     
==========================================
+ Hits         5954     5964      +10     
  Misses       1898     1898              
  Partials      910      910

Impacted Files	Coverage Δ
github/github.go	`89.91% <100.00%> (+0.23%)`	⬆️
github/reactions.go	`64.50% <0.00%> (ø)`
github/actions_runners.go	`51.35% <0.00%> (ø)`
github/issues_comments.go	`78.87% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e881974...bd73ea4. Read the comment docs.

itsksaurabh · 2020-07-16T14:17:41Z

@googlebot I fixed it.

@itsksaurabh

… to reuse the same underlying TCP connection - @itsksaurabh

googlebot · 2020-07-16T15:07:58Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gmlewis

Thank you, @itsksaurabh !
LGTM.

Awaiting second LGTM before merging.

itsksaurabh · 2020-07-23T04:06:58Z

@gmlewis It's my pleasure! :)
Thank you for your approval. 🙏

gmlewis

I'm not sure why this says I haven't reviewed it, which I obviously have in the Conversation thread.

Still awaiting a second LGTM before merging.

philippfranke · 2020-08-11T07:34:06Z

github/github.go

+			io.CopyN(ioutil.Discard, resp.Body, maxBodySlurpSize)
+		}
+
+		if rerr := resp.Body.Close(); err == nil {


I think, this if condition can be removed because err is not part of the named return values. Therefore, the error is never returned if any occur.

Nonetheless, it's a really interesting issue you fixed 👍

@philippfranke Thank you for the review. I am glad that you liked the PR.
Yes, I also agree with your point. There is no need to check for error as it is not part of the named return values. Also, If it fails, the Transport won't reuse it anyway.

@philippfranke I just changed the code. Please let me know if it seems good to you or if some improvements are needed. Thanks! :)

philippfranke

LGTM

itsksaurabh · 2020-08-11T09:48:30Z

Thank you for the approval @philippfranke ♥️

gmlewis · 2020-08-11T12:42:36Z

Thank you, @philippfranke !
Merging.

github/github.go

bvaliev

if resp.ContentLength == -1
not needed here because it's int64 and it is always less than -1 if less than 2048

Checking resp for nil is unnecessary if the code is written according to Go standards because if err != nil is returned then resp must be nil

Generally you had to write resp.ContentLength <= 2048 instead 565 and 566 strings

gmlewis · 2020-11-11T16:32:11Z

Yes, @bvaliev - this PR has issues. PRs are welcome and we'll review proposed changes.

itsksaurabh · 2020-11-12T04:25:33Z

if resp.ContentLength == -1
not needed here because it's int64 and it is always less than -1 if less than 2048

Checking resp for nil is unnecessary if the code is written according to Go standards because if err != nil is returned then resp must be nil

Generally you had to write resp.ContentLength <= 2048 instead 565 and 566 strings

@bvaliev I am not sure about everything you mentioned above. :)

    // ContentLength records the length of the associated content.
    // The value -1 indicates that the length is unknown.
    // Values >= 0 indicate that the given number of bytes may
    // be read from Body.
    //
    // For client requests, a value of 0 with a non-nil Body is
    // also treated as unknown.
    ContentLength int64

https://golang.org/pkg/net/http/

Do you have them somewhere specified in the docs? If so I would love to learn from it. Thanks!

itsksaurabh · 2020-11-12T05:13:58Z

@gmlewis Thank you for your time on this PR. I would like to share with you that the latest official Go Http package does connection reuse internally now in the same way I did in the PR as you could see here. This shows the changes were needed indeed. So now, it is possible to clean up the code.

I have created a PR #1645 for it and now we could happily remove the code. Please have a look :)

googlebot added the cla: no label Jul 16, 2020

Ensure the response body is fully read and closed before we reconnect…

246e4c5

… to reuse the same underlying TCP connection - @itsksaurabh

itsksaurabh force-pushed the master branch from 2b25ea4 to 246e4c5 Compare July 16, 2020 15:07

googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Jul 16, 2020

itsksaurabh changed the title ~~HTTP Client Improvent: Reuse Underlying TCP connection~~ HTTP Client Improvement: Reuse Underlying TCP connection Jul 16, 2020

gmlewis approved these changes Jul 23, 2020

View reviewed changes

gmlewis requested a review from wesleimp July 23, 2020 00:21

itsksaurabh requested a review from gmlewis August 10, 2020 14:59

gmlewis approved these changes Aug 11, 2020

View reviewed changes

philippfranke reviewed Aug 11, 2020

View reviewed changes

remove unnecessary err returns

bd73ea4

philippfranke approved these changes Aug 11, 2020

View reviewed changes

itsksaurabh changed the title ~~HTTP Client Improvement: Reuse Underlying TCP connection~~ HTTP Client Improvement Aug 11, 2020

gmlewis changed the title ~~HTTP Client Improvement~~ Read response body for TCP connection reuse Aug 11, 2020

gmlewis merged commit e19d4dd into google:master Aug 11, 2020

gmlewis reviewed Aug 14, 2020

View reviewed changes

github/github.go Show resolved Hide resolved

gmlewis added the bug label Aug 14, 2020

dmitshur mentioned this pull request Aug 15, 2020

determine if there are situations when io.CopyN is needed to reuse underlying TCP connection #1575

Open

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Read response body for TCP connection reuse (google#1576)

dc8009b

bvaliev reviewed Nov 10, 2020

View reviewed changes

itsksaurabh mentioned this pull request Nov 12, 2020

Clean up connection reuse code inside Do func as now Go HTTP packge does it internally #1645

Merged

itsksaurabh mentioned this pull request Nov 12, 2020

Clean up TCP connection reuse code inside Do func as now Go HTTP packge does it internally #1646

Closed

gmlewis mentioned this pull request Oct 3, 2021

http2 failure in UploadAsset #2113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read response body for TCP connection reuse #1576

Read response body for TCP connection reuse #1576

itsksaurabh commented Jul 16, 2020 •

edited

Loading

googlebot commented Jul 16, 2020

codecov bot commented Jul 16, 2020 •

edited

Loading

itsksaurabh commented Jul 16, 2020

googlebot commented Jul 16, 2020

gmlewis left a comment

itsksaurabh commented Jul 23, 2020

gmlewis left a comment

philippfranke Aug 11, 2020 •

edited

Loading

itsksaurabh Aug 11, 2020

itsksaurabh Aug 11, 2020 •

edited

Loading

philippfranke left a comment

itsksaurabh commented Aug 11, 2020

gmlewis commented Aug 11, 2020

bvaliev left a comment •

edited

Loading

gmlewis commented Nov 11, 2020

itsksaurabh commented Nov 12, 2020

itsksaurabh commented Nov 12, 2020 •

edited

Loading

Read response body for TCP connection reuse #1576

Read response body for TCP connection reuse #1576

Conversation

itsksaurabh commented Jul 16, 2020 • edited Loading

googlebot commented Jul 16, 2020

codecov bot commented Jul 16, 2020 • edited Loading

Codecov Report

itsksaurabh commented Jul 16, 2020

googlebot commented Jul 16, 2020

gmlewis left a comment

Choose a reason for hiding this comment

itsksaurabh commented Jul 23, 2020

gmlewis left a comment

Choose a reason for hiding this comment

philippfranke Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

itsksaurabh Aug 11, 2020

Choose a reason for hiding this comment

itsksaurabh Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

philippfranke left a comment

Choose a reason for hiding this comment

itsksaurabh commented Aug 11, 2020

gmlewis commented Aug 11, 2020

bvaliev left a comment • edited Loading

Choose a reason for hiding this comment

gmlewis commented Nov 11, 2020

itsksaurabh commented Nov 12, 2020

itsksaurabh commented Nov 12, 2020 • edited Loading

itsksaurabh commented Jul 16, 2020 •

edited

Loading

codecov bot commented Jul 16, 2020 •

edited

Loading

philippfranke Aug 11, 2020 •

edited

Loading

itsksaurabh Aug 11, 2020 •

edited

Loading

bvaliev left a comment •

edited

Loading

itsksaurabh commented Nov 12, 2020 •

edited

Loading