Connections are not kept-alive about goamz HOT 8 CLOSED

Does this have a measurable impact on performance? or memory use? I guess the actual HTTP connections to S3 servers are closed after each call on the sever side, right?

from goamz.

It's difficult to benchmark because of S3's performance fluctuations, but the theory is that reusing connections will reduce latency on subsequent requests.

The documentation says: The Client's Transport typically has internal state (cached TCP connections), so Clients should be reused instead of created as needed.

http://golang.org/pkg/net/http/#Client

from goamz.

It's possible that this is significant/measurable for HTTPS communication (all requests from the s3 package are HTTPS, yes?). AFAIK each connection will initiate a new TLS handshake. The handshake involves back and forth communication that (in my experiences working with HTTP libraries in other languages) adds a significant amount of time to the request.

from goamz.

The Go 1.3 http.Client has a Timeout field that can effectively replace the way ReadTimeout is implemented. If that's changed the http.Client (and its underlying tcp connections) should be reusable across multiple calls to s3.run() (the current implementation appears to have deadlines kick in on subsequent requests using the same underlying tcp connection, closing them and causing an error).

I'm willing to write a patch for this.

from goamz.

Go for it man!

from goamz.

Cool.

After playing around with it a little yesterday I'm considering removing the ConnectTimeout and ReadTimeout fields from S3 and replacing them with a public *http.Client field. That would also allow a proper workaround for #127.

I think it's ok. Implementing the timeouts in 1.3 is more straightword than what is being done currently. The net.Dialer type can be used now that SetDeadline isn't needed.

&http.Client{
    Transport: &http.Transport{
        Dial: (&net.Dialer{
            Timeout:   connect,
            KeepAlive: 30 * time.Second,
        }).Dial,
    },
    Timeout: read,
}

There could be a public function in the s3 package that constructs and returns a client like above. I don't have a very strong opinion about that.

How does this sound?

from goamz.

ping @alimoeeny

I have been thinking, instead of doing what I described above, the exported API could stay the same (with timeout fields and an unexported http client) if the methods I suggest in #169 are introduced. It allows the user to completely bypass the http client used by the S3 type. IMO, this is also an acceptable workaround for #127. At the moment I'm leaning toward this solution because it is backwards compatible.

Unless there are other ideas I will start forming a pull request implementing these changes.

from goamz.

Thanks @bmatsuo ,
IMHO, keeping things backward compatible is very important.
If we can have a default behavior that is good enough for the majority, which could be overridden when somebody needs better performance or finer control, that would be the best.
Thanks again,

from goamz.