http: persistent client connections #52

Merged
merged 1 commit into from Jul 5, 2017

Conversation

Projects
None yet
5 participants
Member

rogpeppe commented Jul 5, 2017

Two issues were preventing client connections from being
reused (leading to significant performance reduction):

  • we were specifically asking for connections not to be reused,
    citing Go issue 4677, which has been fixed since Go 1.6

  • Some responses (notably the response to CreateContainer)
    were returning response bodies which were not being read.

http: persistent client connections
Two issues were preventing client connections from being
reused (leading to significant performance reduction):

- we were specifically asking for connections not to be reused,
citing Go issue 4677, which has been fixed since Go 1.6

- Some responses (notably the response to CreateContainer)
were returning response bodies which were not being read.

Looks good. Please add license headers to the two new files.

Assuming you've tested this live, since I'm sure that's how we encountered issue 4677 originally.

+ if size > 1024 || size < 0 {
+ size = 1024
+ }
+ resp.Body.Read(make([]byte, size))
@jameinel

jameinel Jul 5, 2017

Owner

Don't you have to read all of ContentLength for the connection to be ready for the next response?

@rogpeppe

rogpeppe Jul 5, 2017

Member

Yes, but I don't think it's worth reading large response bodies to save a connection. In the particular case we're seeing in swift, the body is only about 100 bytes. We could tweak this value later if we find it's not effective.

+ } {
+ assertReq(path, fmt.Sprint(i))
+ }
+ c.Assert(connCount, gc.Equals, int32(1))
@jameinel

jameinel Jul 5, 2017

Owner

if you're going to use atomic to AddInt, don't you have to use LoadInt to be sure this thread gets the right content?

@rogpeppe

rogpeppe Jul 5, 2017

Member

I'm assuming that connections won't be made without an HTTP request to stimulate them. The atomic.Add is probably overkill, tbh, as I can't see net/http making two concurrent connections with only one request. If there really is a danger of the connection count incrementing asynchronously after all the connections are made, I think I'd prefer the race detector to complain.

Member

rogpeppe commented Jul 5, 2017

$$merge$$

Member

mhilton commented Jul 5, 2017

$$merge$$

Member

jujubot commented Jul 5, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-goose

@jujubot jujubot merged commit 5acf2c9 into go-goose:v2 Jul 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment