PutObject with `io.Reader` which doesn't implement `Seek` always retries but fails

[moustafab]

When you use PutObject with an io.Reader which doesn't implement Seek (i.e. not a io.ReadSeeker) like bytes.Buffer, if you receive any kind of retry-able error on uploading the object you would see errors like:

net/http: HTTP/1.x transport connection broken: http: ContentLength=XXX with Body length 0

As far as I can tell this is because the Progress option even when nil is used to create a hookReader wrapping the original io.Reader which does implement io.ReadSeeker and results in attempts to Seek using the fall through logic of the hookReader which if wrapping a source which doesn't implement io.Seeker cannot actually seek to the appropriate offset (usually the beginning) used in the retry logic.

I took a look and I believe the original intention was to just disallow retries for io.Reader input which doesn't implement io.Seeker but with the progress wrapper, effectively the code path that disallows retries for non-io.Seeker implementations does not ever get triggered.

I think there are 3 problems here:

1. It's not clear to a user (while admittedly being somewhat obvious after thinking about it for a bit) that using a non-io.Seeker as input should result in no retries.
2. Using an input that doesn't implement io.Seeker in fact does result in a retry which could never succeed and throws the very cryptic error message mentioned above.
3. Using the SendContentMd5 option wraps any io.Reader which doesn't implement io.Seeker with a io.ReadSeeker implementation which effectively papers over the problem as a side-effect.

Reproduction example:

Details

// This code has to be run against something that will serve up a retry-able error for the PutObject calls to demonstrate the problem.

// this will retry the upload operation
func UploadWithReadSeeker(client *minio.Client, bucketName string, rawData []byte) error {
	readSeeker, size := bytes.NewReader(rawData), int64(len(rawData))
	_, err := client.PutObject(context.Background(), bucketName, "with-read-seeker", readSeeker, size, minio.PutObjectOptions{})
	return errors.Wrap(err, "failed to upload object")
}

// this will attempt to retry the upload operation and immediately fail
func UploadWithReader(client *minio.Client, bucketName string, rawData []byte) error {
	reader, size := bytes.NewBuffer(rawData), int64(len(rawData))
	_, err := client.PutObject(context.Background(), bucketName, "with-reader", reader, size, minio.PutObjectOptions{})
	return errors.Wrap(err, "failed to upload object")
}

// this will retry the upload operation
func UploadWithReaderWMd5Hash(client *minio.Client, bucketName string, rawData []byte) error {
	reader, size := bytes.NewBuffer(rawData), int64(len(rawData))
	_, err := client.PutObject(context.Background(), bucketName, "with-reader", reader, size, minio.PutObjectOptions{
		SendContentMd5: true,
	})
	return errors.Wrap(err, "failed to upload object")
}

Investigation notes:

Perhaps introduced accidentally in #1673
Having and not having progress hook shouldn't really influence the retry behavior. Perhaps it's best to implement two hookReader types, one which is a pure one-shot io.Reader and another which is a io.ReadSeeker and match according to the input type?

[harshavardhana]

Thanks for the reproducer will take a look.