Always read HTTP response body in case of an error

[fd0]

This allows reusing the same connection for future requests. A connection is aborted when the body is closed before reading it completely.

[fd0]

As discussed on Slack, these two commits resolve all instances of non-drained HTTP response bodies, I'm not seeing any aborted connections any more.

[fd0]

Hm, feels like a flaky test. Anything else I can do?

[ncw]

I gave the test a kick to see if it runs through OK and it did.

As for the logic - I'm a bit uneasy about replacing all the checkClose calls.

In particular the one in ObjectGet - callers are quite entitled to read the object for a bit then close it (rclone does exactly this when seeking objects in its fuse FS). Having checkClose read the entire buffer here of a 5GB file would be unwise.

My suggestion would be to leave checkClose alone and make a new drainCheckClose then we can closely audit where those drainCheckClose get used.

[ncw]

Sorry it is the one in func (file *ObjectOpenFile) Close that we should be worrying about

[fd0]

sounds good, I'll rework it

[cezarsa]

If I may chime in, I share @ncw worries on this and I think a similar situation can happen with the call to checkClose in (*Connection).ObjectGet. Suppose I have a code for a external timeout that looks something like this:

done := make(chan bool)
go func() {
    defer close(done)
    conn.ObjectGet("c", "o", otherConn, false, nil)
}()
select {
case <-time.After(time.Minute):
    otherConn.Close()
case <-done:
}

It's not ideal, but certainly possible, and I would expect that after closing the writer no further data would be read from this object as the io.Copy would eventually fail. I think it would be better to drop this connection than potentially keep reading lots of data from a slow connection.

[fd0]

Uh, that is an excellent point, thanks for bringing it up! Any idea on what to do now?

[cezarsa]

I think the same thing @ncw suggested would be fine, keepcheckClose as is and create a new func drainCheckClose. This new func would replace checkClose almost everywhere with the exception of (*ObjectOpenFile).Close and (*Connection).ObjectGet.

[fd0]

Done, let me know what you think :)

[fd0]

Hm, the flaky test again...

[fd0]

So, anything else I can do here?

[cezarsa]

Thank you @fd0 !