Merge lp:~niemeyer/goamz/put-all into lp:~gophers/goamz/trunk

[Gustavo Niemeyer]

s3: Multi.PutAll for auto-chunking and resuming

Bucket.Multi method also added for trivial resume-or-init handling.

https://codereview.appspot.com/7313081/

[Gustavo Niemeyer (niemeyer)]

Reviewers: mp+147829_code.launchpad.net,

Message:
Please take a look.

Description:
s3: Multi.PutAll for auto-chunking and resuming

Bucket.Multi method also added for trivial resume-or-init handling.

https://code.launchpad.net/~niemeyer/goamz/put-all/+merge/147829

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7313081/

Affected files:
   A [revision details]
   M s3/multi.go
   M s3/multi_test.go
   M s3/responses_test.go
   M s3/s3_test.go
   M s3/s3i_test.go

[John A Meinel (jameinel)]

LGTM

https://codereview.appspot.com/7313081/diff/1/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode272
s3/multi.go:272: // the new part, or otherwise overwritten with the new
content.
does partSize need to match exactly the pre-uploaded content? I don't
see places where that is checked here. Probably at least a doc comment
about it.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
What will this return if totalSize is 0? the return of ListParts? But
doesn't that violate the 'you should return nil, otherwise the caller
gets an interface object that contains nil but isn't nil'?

So I would probably split this:
if err != nil {
   return nil, err
} else if totalSize == 0 {
   return nil, nil
}

Though since you don't have a result object, wouldn't you want a non-nil
err? Otherwise you'll get a segfault accessing the result later.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode310
s3/multi.go:310: part, err := m.putPart(current, section, partSize,
md5b64)
As an aside, would there be any reason to do parallel uploads?

https://codereview.appspot.com/7313081/

[Dimiter Naydenov (dimitern)]

LGTM, a few suggestions.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode227
s3/multi.go:227: var listPartsMax = 1000
Is this an absolute limit of all parts per client connection?

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
On 2013/02/12 07:32:38, jameinel wrote:
> What will this return if totalSize is 0? the return of ListParts? But
doesn't
> that violate the 'you should return nil, otherwise the caller gets an
interface
> object that contains nil but isn't nil'?

> So I would probably split this:
> if err != nil {
> return nil, err
> } else if totalSize == 0 {
> return nil, nil
> }

> Though since you don't have a result object, wouldn't you want a
non-nil err?
> Otherwise you'll get a segfault accessing the result later.

ISTM if totalSize == 0 it will return nil, nil, since there won't be an
error. But if it's not obvious, +1 on the suggestion.

https://codereview.appspot.com/7313081/diff/1/s3/multi_test.go
File s3/multi_test.go (right):

https://codereview.appspot.com/7313081/diff/1/s3/multi_test.go#newcode53
s3/multi_test.go:53: c.Assert(multi.UploadId, Matches,
"JNbR_[A-Za-z0-9.]+QQ--")
Maybe move this in assertId()? This seems to be used multiple times, so
it'll be better to have it at one place, I think.

https://codereview.appspot.com/7313081/

[Gustavo Niemeyer (niemeyer)]

Thanks for the reviews.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode227
s3/multi.go:227: var listPartsMax = 1000
On 2013/02/12 08:02:52, dimitern wrote:
> Is this an absolute limit of all parts per client connection?

No, this is a per-request limit, and 1000 is the default limit as well.
It's here as a variable just so we can test it. I'll add a comment.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode272
s3/multi.go:272: // the new part, or otherwise overwritten with the new
content.
On 2013/02/12 07:32:38, jameinel wrote:
> does partSize need to match exactly the pre-uploaded content? I don't
see places
> where that is checked here. Probably at least a doc comment about it.

It's actually explicitly checked, but that's a bit redundant with the
above statement. If the size changes, the checksum won't match. That
said, given that you felt the need to ask, it won't hurt to mention it
explicitly.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
On 2013/02/12 07:32:38, jameinel wrote:
> So I would probably split this:
> if err != nil {
> return nil, err
> } else if totalSize == 0 {
> return nil, nil
> }

Hmm.. that does exactly the same thing that the current version does.

> Though since you don't have a result object, wouldn't you want a
non-nil err?
> Otherwise you'll get a segfault accessing the result later.

That's not the case with slices. The most conventional way to represent
an empty slice is with a nil/zero value. len, append, etc, all work
properly with them, and code taking slices should behave properly as
well, or may otherwise be considered broken.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode310
s3/multi.go:310: part, err := m.putPart(current, section, partSize,
md5b64)
On 2013/02/12 07:32:38, jameinel wrote:
> As an aside, would there be any reason to do parallel uploads?

That's an interesting question. I guess we could probably get some speed
up by sending parts in parallel. It's a good topic to do some
experimentation on.

https://codereview.appspot.com/7313081/

[John A Meinel (jameinel)]

LGTM

https://codereview.appspot.com/7313081/diff/1/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
That's fine. I wasn't thinking about it being a slice, it does seem
correct there.

The concern was stuff like: http://play.golang.org/p/ZcjGU2BgbC

But that doesn't apply here because err has type "error" rather than a
custom type that could be a nil pointer.

https://codereview.appspot.com/7313081/

[Roger Peppe (rogpeppe)]

really nice API.
a few suggestions below.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode88
s3/multi.go:88: // inside b. If a multipart upload exists for key, it is
returned,
if we assume that a bucket implies its key, then i think the explanation
can become simpler. how about this, perhaps?

// Multi returns a multipart upload handler for the bucket.
// If one already exists, it is returned, otherwise
// a new multipart upload is initiated with contType and perm.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode229
s3/multi.go:229: // ListParts returns the list of previously uploaded
parts in m.
// ordered by part number.
?

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode269
s3/multi.go:269: // PutAll sends all of r via a multipart upload with
parts no large
s/large/larger/

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode273
s3/multi.go:273: // PutAll returns all the parts of m (reused or not).
nice interface.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode279
s3/multi.go:279: reuse := 1 // Index of next old part to consider
reusing.
I was a little confused here by the one-based indexing used for reuse
here - it makes it look like it's in the same units as current, but
reuse really is an index (into old) and current is a part number.

Also, current isn't really the index of the last good part handled,
AFAICS - it's the part number of the part we're about to consider
putting.

I'd be inclined to do:

reuse := 0 // Index of next old part to consider reusing.
current := 1 // Part number of current part.

and adjust the use of reuse below.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
On 2013/02/12 08:02:52, dimitern wrote:
> On 2013/02/12 07:32:38, jameinel wrote:
> > What will this return if totalSize is 0? the return of ListParts?
But doesn't
> > that violate the 'you should return nil, otherwise the caller gets
an
> interface
> > object that contains nil but isn't nil'?
> >
> > So I would probably split this:
> > if err != nil {
> > return nil, err
> > } else if totalSize == 0 {
> > return nil, nil
> > }
> >
> > Though since you don't have a result object, wouldn't you want a
non-nil err?
> > Otherwise you'll get a segfault accessing the result later.

> ISTM if totalSize == 0 it will return nil, nil, since there won't be
an error.
> But if it's not obvious, +1 on the suggestion.

i don't mind the way it's phrased here, but i would like to see a test
case that tests putting a zero-length file live (i *presume* you can
read a file ok if it's had no PUT requests, but it's only a
presumption). perhaps i missed a relevant test though.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode316
s3/multi.go:316: }
the logic above is moderately intricate - i'd like to see some more test
cases to go with TestPutAllResume,
at least a case with a reused part in the middle of other non-reused
parts, and vice versa.

https://codereview.appspot.com/7313081/

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/7313081/diff/1/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode88
s3/multi.go:88: // inside b. If a multipart upload exists for key, it is
returned,
On 2013/02/13 10:22:03, rog wrote:
> if we assume that a bucket implies its key, then i think the
explanation can
> become simpler. how about this, perhaps?

> // Multi returns a multipart upload handler for the bucket.
> // If one already exists, it is returned, otherwise
> // a new multipart upload is initiated with contType and perm.

A bucket doesn't imply its key. A bucket contains keys. What's described
above is not the same as what was documented in the method, or what the
method does.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode229
s3/multi.go:229: // ListParts returns the list of previously uploaded
parts in m.
On 2013/02/13 10:22:03, rog wrote:
> // ordered by part number.
> ?

Sounds good.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode269
s3/multi.go:269: // PutAll sends all of r via a multipart upload with
parts no large
On 2013/02/13 10:22:03, rog wrote:
> s/large/larger/

Thanks.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode279
s3/multi.go:279: reuse := 1 // Index of next old part to consider
reusing.
On 2013/02/13 10:22:03, rog wrote:
> I was a little confused here by the one-based indexing used for reuse
here - it
> makes it look like it's in the same units as current, but reuse really
is an
> index (into old) and current is a part number.

> Also, current isn't really the index of the last good part handled,
AFAICS -
> it's the part number of the part we're about to consider putting.

> I'd be inclined to do:

> reuse := 0 // Index of next old part to consider reusing.
> current := 1 // Part number of current part.

> and adjust the use of reuse below.

Sounds good.

https://codereview.appspot.com/7313081/diff/1/s3/multi.go#newcode282
s3/multi.go:282: if totalSize == 0 || err != nil {
On 2013/02/13 10:22:03, rog wrote:
> On 2013/02/12 08:02:52, dimitern wrote:
> > On 2013/02/12 07:32:38, jameinel wrote:
> > > What will this return if totalSize is 0? the return of ListParts?
But
> doesn't
> > > that violate the 'you should return nil, otherwise the caller gets
an
> > interface
> > > object that contains nil but isn't nil'?
> > >
> > > So I would probably split this:
> > > if err != nil {
> > > return nil, err
> > > } else if totalSize == 0 {
> > > return nil, nil
> > > }
> > >
> > > Though since you don't have a result object, wouldn't you want a
non-nil
> err?
> > > Otherwise you'll get a segfault accessing the result later.
> >
> > ISTM if totalSize == 0 it will return nil, nil, since there won't be
an error.
> > But if it's not obvious, +1 on the suggestion.

> i don't mind the way it's phrased here, but i would like to see a test
case that
> tests putting a zero-length file live (i *presume* you can read a file
ok if
> it's had no PUT requests, but it's only a presumption). perhaps i
missed a
> relevant test though.

I don't think it matters whether that works or not in the server. That's
an edge case, and the ser...

[lp:~niemeyer/goamz/put-all]

32. By Gustavo Niemeyer on 2013-02-13

Changes after reviews.

[Gustavo Niemeyer (niemeyer)]

Please take a look.

https://codereview.appspot.com/7313081/

[Roger Peppe (rogpeppe)]

LGTM modulo the below.

https://codereview.appspot.com/7313081/diff/8001/s3/multi.go
File s3/multi.go (right):

https://codereview.appspot.com/7313081/diff/8001/s3/multi.go#newcode276
s3/multi.go:276: // new content.
As discussed, perhaps a comment like:
// Multipart uploads of zero length files may not work.

https://codereview.appspot.com/7313081/diff/8001/s3/multi.go#newcode284
s3/multi.go:284: current := 1 // Index of latest good part handled.
This comment is still misleading. Current isn't an index in the same way
that reuse is, and at this point we haven't any good parts, so there's
no "latest" good part - current is the part number of the next part to
handle.

https://codereview.appspot.com/7313081/

[lp:~niemeyer/goamz/put-all]

33. By Gustavo Niemeyer on 2013-02-13

Add edge case for zero-length files.

[Gustavo Niemeyer (niemeyer)]

Please take a look.

https://codereview.appspot.com/7313081/

[Gustavo Niemeyer (niemeyer)]

*** Submitted:

s3: Multi.PutAll for auto-chunking and resuming

Bucket.Multi method also added for trivial resume-or-init handling.

R=jameinel, dimitern, rog
CC=
https://codereview.appspot.com/7313081

https://codereview.appspot.com/7313081/

=== modified file 's3/multi.go'
--- s3/multi.go	2013-02-05 22:56:23 +0000
+++ s3/multi.go	2013-02-13 12:22:21 +0000
@@ -4,6 +4,7 @@
 	"bytes"
 	"crypto/md5"
 	"encoding/base64"
+	"encoding/hex"
 	"encoding/xml"
 	"errors"
 	"io"
@@ -24,6 +25,7 @@
 	UploadId string
 }
 
+// That's the default. Here just for testing.
 var listMultiMax = 1000
 
 type listMultiResp struct {
@@ -83,6 +85,22 @@
 	panic("unreachable")
 }
 
+// Multi returns a multipart upload handler for the provided key
+// inside b. If a multipart upload exists for key, it is returned,
+// otherwise a new multipart upload is initiated with contType and perm.
+func (b *Bucket) Multi(key, contType string, perm ACL) (*Multi, error) {
+	multis, _, err := b.ListMulti(key, "")
+	if err != nil && !hasCode(err, "NoSuchUpload") {
+		return nil, err
+	}
+	for _, m := range multis {
+		if m.Key == key {
+			return m, nil
+		}
+	}
+	return b.InitMulti(key, contType, perm)
+}
+
 // InitMulti initializes a new multipart upload at the provided
 // key inside b and returns a value for manipulating it.
 //
@@ -124,13 +142,17 @@
 //
 // See http://goo.gl/pqZer for details.
 func (m *Multi) PutPart(n int, r io.ReadSeeker) (Part, error) {
-	length, b64md5, err := seekerInfo(r)
+	partSize, _, md5b64, err := seekerInfo(r)
 	if err != nil {
 		return Part{}, err
 	}
+	return m.putPart(n, r, partSize, md5b64)
+}
+
+func (m *Multi) putPart(n int, r io.ReadSeeker, partSize int64, md5b64 string) (Part, error) {
 	headers := map[string][]string{
-		"Content-Length": {strconv.FormatInt(length, 10)},
-		"Content-MD5":    {b64md5},
+		"Content-Length": {strconv.FormatInt(partSize, 10)},
+		"Content-MD5":    {md5b64},
 	}
 	params := map[string][]string{
 		"uploadId":   {m.UploadId},
@@ -164,23 +186,25 @@
 		if etag == "" {
 			return Part{}, errors.New("part upload succeeded with no ETag")
 		}
-		return Part{n, etag, length}, nil
+		return Part{n, etag, partSize}, nil
 	}
 	panic("unreachable")
 }
 
-func seekerInfo(r io.ReadSeeker) (length int64, b64md5 string, err error) {
+func seekerInfo(r io.ReadSeeker) (size int64, md5hex string, md5b64 string, err error) {
 	_, err = r.Seek(0, 0)
 	if err != nil {
-		return 0, "", err
+		return 0, "", "", err
 	}
 	digest := md5.New()
-	length, err = io.Copy(digest, r)
+	size, err = io.Copy(digest, r)
 	if err != nil {
-		return 0, "", err
+		return 0, "", "", err
 	}
-	b64md5 = base64.StdEncoding.EncodeToString(digest.Sum(nil))
-	return length, b64md5, nil
+	sum := digest.Sum(nil)
+	md5hex = hex.EncodeToString(sum)
+	md5b64 = base64.StdEncoding.EncodeToString(sum)
+	return size, md5hex, md5b64, nil
 }
 
 type Part struct {
@@ -189,15 +213,23 @@
 	Size int64
 }
 
+type partSlice []Part
+
+func (s partSlice) Len() int           { return len(s) }
+func (s partSlice) Less(i, j int) bool { return s[i].N < s[j].N }
+func (s partSlice) Swap(i, j int)      { s[i], s[j] = s[j], s[i] }
+
 type listPartsResp struct {
 	NextPartNumberMarker string
 	IsTruncated          bool
 	Part                 []Part
 }
 
+// That's the default. Here just for testing.
 var listPartsMax = 1000
 
-// ListParts returns the list of previously uploaded parts in m.
+// ListParts returns the list of previously uploaded parts in m,
+// ordered by part number.
 //
 // See http://goo.gl/ePioY for details.
 func (m *Multi) ListParts() ([]Part, error) {
@@ -205,7 +237,7 @@
 		"uploadId":  {m.UploadId},
 		"max-parts": {strconv.FormatInt(int64(listPartsMax), 10)},
 	}
-	var parts []Part
+	var parts partSlice
 	for attempt := attempts.Start(); attempt.Next(); {
 		req := &request{
 			method: "GET",
@@ -223,6 +255,7 @@
 		}
 		parts = append(parts, resp.Part...)
 		if !resp.IsTruncated {
+			sort.Sort(parts)
 			return parts, nil
 		}
 		params["part-number-marker"] = []string{resp.NextPartNumberMarker}
@@ -231,6 +264,65 @@
 	panic("unreachable")
 }
 
+type ReaderAtSeeker interface {
+	io.ReaderAt
+	io.ReadSeeker
+}
+
+// PutAll sends all of r via a multipart upload with parts no larger
+// than partSize bytes, which must be set to at least 5MB.
+// Parts previously uploaded are either reused if their checksum
+// and size match the new part, or otherwise overwritten with the
+// new content.
+// PutAll returns all the parts of m (reused or not).
+func (m *Multi) PutAll(r ReaderAtSeeker, partSize int64) ([]Part, error) {
+	old, err := m.ListParts()
+	if err != nil && !hasCode(err, "NoSuchUpload") {
+		return nil, err
+	}
+	reuse := 0   // Index of next old part to consider reusing.
+	current := 1 // Part number of latest good part handled.
+	totalSize, err := r.Seek(0, 2)
+	if err != nil {
+		return nil, err
+	}
+	first := true // Must send at least one empty part if the file is empty.
+	var result []Part
+NextSection:
+	for offset := int64(0); offset < totalSize || first; offset += partSize {
+		first = false
+		if offset+partSize > totalSize {
+			partSize = totalSize - offset
+		}
+		section := io.NewSectionReader(r, offset, partSize)
+		_, md5hex, md5b64, err := seekerInfo(section)
+		if err != nil {
+			return nil, err
+		}
+		for reuse < len(old) && old[reuse].N <= current {
+			// Looks like this part was already sent.
+			part := &old[reuse]
+			etag := `"` + md5hex + `"`
+			if part.N == current && part.Size == partSize && part.ETag == etag {
+				// Checksum matches. Reuse the old part.
+				result = append(result, *part)
+				current++
+				continue NextSection
+			}
+			reuse++
+		}
+
+		// Part wasn't found or doesn't match. Send it.
+		part, err := m.putPart(current, section, partSize, md5b64)
+		if err != nil {
+			return nil, err
+		}
+		result = append(result, part)
+		current++
+	}
+	return result, nil
+}
+
 type completeUpload struct {
 	XMLName xml.Name      `xml:"CompleteMultipartUpload"`
 	Parts   completeParts `xml:"Part"`
@@ -298,14 +390,14 @@
 // See http://goo.gl/dnyJw for details.
 func (m *Multi) Abort() error {
 	params := map[string][]string{
-		"uploadId":   {m.UploadId},
+		"uploadId": {m.UploadId},
 	}
 	for attempt := attempts.Start(); attempt.Next(); {
 		req := &request{
-			method:  "DELETE",
-			bucket:  m.Bucket.Name,
-			path:    m.Key,
-			params:  params,
+			method: "DELETE",
+			bucket: m.Bucket.Name,
+			path:   m.Key,
+			params: params,
 		}
 		err := m.Bucket.S3.query(req, nil)
 		if shouldRetry(err) && attempt.HasNext() {

=== modified file 's3/multi_test.go'
--- s3/multi_test.go	2013-02-01 05:58:42 +0000
+++ s3/multi_test.go	2013-02-13 12:22:21 +0000
@@ -2,12 +2,13 @@
 
 import (
 	"encoding/xml"
+	"io"
+	"io/ioutil"
 	"launchpad.net/goamz/s3"
 	. "launchpad.net/gocheck"
 	"strings"
 )
 
-
 func (s *S) TestInitMulti(c *C) {
 	testServer.Response(200, nil, InitMultiResultDump)
 
@@ -26,6 +27,88 @@
 	c.Assert(multi.UploadId, Matches, "JNbR_[A-Za-z0-9.]+QQ--")
 }
 
+func (s *S) TestMultiNoPreviousUpload(c *C) {
+	// Don't retry the NoSuchUpload error.
+	s.DisableRetries()
+
+	testServer.Response(404, nil, NoSuchUploadErrorDump)
+	testServer.Response(200, nil, InitMultiResultDump)
+
+	b := s.s3.Bucket("sample")
+
+	multi, err := b.Multi("multi", "text/plain", s3.Private)
+	c.Assert(err, IsNil)
+
+	req := testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "GET")
+	c.Assert(req.URL.Path, Equals, "/sample/")
+	c.Assert(req.Form["uploads"], DeepEquals, []string{""})
+	c.Assert(req.Form["prefix"], DeepEquals, []string{"multi"})
+
+	req = testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "POST")
+	c.Assert(req.URL.Path, Equals, "/sample/multi")
+	c.Assert(req.Form["uploads"], DeepEquals, []string{""})
+
+	c.Assert(multi.UploadId, Matches, "JNbR_[A-Za-z0-9.]+QQ--")
+}
+
+func (s *S) TestMultiReturnOld(c *C) {
+	testServer.Response(200, nil, ListMultiResultDump)
+
+	b := s.s3.Bucket("sample")
+
+	multi, err := b.Multi("multi1", "text/plain", s3.Private)
+	c.Assert(err, IsNil)
+	c.Assert(multi.Key, Equals, "multi1")
+	c.Assert(multi.UploadId, Equals, "iUVug89pPvSswrikD")
+
+	req := testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "GET")
+	c.Assert(req.URL.Path, Equals, "/sample/")
+	c.Assert(req.Form["uploads"], DeepEquals, []string{""})
+	c.Assert(req.Form["prefix"], DeepEquals, []string{"multi1"})
+}
+
+func (s *S) TestListParts(c *C) {
+	testServer.Response(200, nil, InitMultiResultDump)
+	testServer.Response(200, nil, ListPartsResultDump1)
+	testServer.Response(404, nil, NoSuchUploadErrorDump) // :-(
+	testServer.Response(200, nil, ListPartsResultDump2)
+
+	b := s.s3.Bucket("sample")
+
+	multi, err := b.InitMulti("multi", "text/plain", s3.Private)
+	c.Assert(err, IsNil)
+
+	parts, err := multi.ListParts()
+	c.Assert(err, IsNil)
+	c.Assert(parts, HasLen, 3)
+	c.Assert(parts[0].N, Equals, 1)
+	c.Assert(parts[0].Size, Equals, int64(5))
+	c.Assert(parts[0].ETag, Equals, `"ffc88b4ca90a355f8ddba6b2c3b2af5c"`)
+	c.Assert(parts[1].N, Equals, 2)
+	c.Assert(parts[1].Size, Equals, int64(5))
+	c.Assert(parts[1].ETag, Equals, `"d067a0fa9dc61a6e7195ca99696b5a89"`)
+	c.Assert(parts[2].N, Equals, 3)
+	c.Assert(parts[2].Size, Equals, int64(5))
+	c.Assert(parts[2].ETag, Equals, `"49dcd91231f801159e893fb5c6674985"`)
+	testServer.WaitRequest()
+	req := testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "GET")
+	c.Assert(req.URL.Path, Equals, "/sample/multi")
+	c.Assert(req.Form.Get("uploadId"), Matches, "JNbR_[A-Za-z0-9.]+QQ--")
+	c.Assert(req.Form["max-parts"], DeepEquals, []string{"1000"})
+
+	testServer.WaitRequest() // The internal error.
+	req = testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "GET")
+	c.Assert(req.URL.Path, Equals, "/sample/multi")
+	c.Assert(req.Form.Get("uploadId"), Matches, "JNbR_[A-Za-z0-9.]+QQ--")
+	c.Assert(req.Form["max-parts"], DeepEquals, []string{"1000"})
+	c.Assert(req.Form["part-number-marker"], DeepEquals, []string{"2"})
+}
+
 func (s *S) TestPutPart(c *C) {
 	headers := map[string]string{
 		"ETag": `"26f90efd10d614f100252ff56d88dad8"`,
@@ -54,44 +137,153 @@
 	c.Assert(req.Header["Content-Md5"], DeepEquals, []string{"JvkO/RDWFPEAJS/1bYja2A=="})
 }
 
-func (s *S) TestListParts(c *C) {
+func readAll(r io.Reader) string {
+	data, err := ioutil.ReadAll(r)
+	if err != nil {
+		panic(err)
+	}
+	return string(data)
+}
+
+func (s *S) TestPutAllNoPreviousUpload(c *C) {
+	// Don't retry the NoSuchUpload error.
+	s.DisableRetries()
+
+	etag1 := map[string]string{"ETag": `"etag1"`}
+	etag2 := map[string]string{"ETag": `"etag2"`}
+	etag3 := map[string]string{"ETag": `"etag3"`}
+	testServer.Response(200, nil, InitMultiResultDump)
+	testServer.Response(404, nil, NoSuchUploadErrorDump)
+	testServer.Response(200, etag1, "")
+	testServer.Response(200, etag2, "")
+	testServer.Response(200, etag3, "")
+
+	b := s.s3.Bucket("sample")
+
+	multi, err := b.InitMulti("multi", "text/plain", s3.Private)
+	c.Assert(err, IsNil)
+
+	parts, err := multi.PutAll(strings.NewReader("part1part2last"), 5)
+	c.Assert(parts, HasLen, 3)
+	c.Assert(parts[0].ETag, Equals, `"etag1"`)
+	c.Assert(parts[1].ETag, Equals, `"etag2"`)
+	c.Assert(parts[2].ETag, Equals, `"etag3"`)
+	c.Assert(err, IsNil)
+
+	// Init
+	testServer.WaitRequest()
+
+	// List old parts. Won't find anything.
+	req := testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "GET")
+	c.Assert(req.URL.Path, Equals, "/sample/multi")
+
+	// Send part 1.
+	req = testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "PUT")
+	c.Assert(req.URL.Path, Equals, "/sample/multi")
+	c.Assert(req.Form["partNumber"], DeepEquals, []string{"1"})
+	c.Assert(req.Header["Content-Length"], DeepEquals, []string{"5"})
+	c.Assert(readAll(req.Body), Equals, "part1")
+
+	// Send part 2.
+	req = testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "PUT")
+	c.Assert(req.URL.Path, Equals, "/sample/multi")
+	c.Assert(req.Form["partNumber"], DeepEquals, []string{"2"})
+	c.Assert(req.Header["Content-Length"], DeepEquals, []string{"5"})
+	c.Assert(readAll(req.Body), Equals, "part2")
+
+	// Send part 3 with shorter body.
+	req = testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "PUT")
+	c.Assert(req.URL.Path, Equals, "/sample/multi")
+	c.Assert(req.Form["partNumber"], DeepEquals, []string{"3"})
+	c.Assert(req.Header["Content-Length"], DeepEquals, []string{"4"})
+	c.Assert(readAll(req.Body), Equals, "last")
+}
+
+func (s *S) TestPutAllZeroSizeFile(c *C) {
+	// Don't retry the NoSuchUpload error.
+	s.DisableRetries()
+
+	etag1 := map[string]string{"ETag": `"etag1"`}
+	testServer.Response(200, nil, InitMultiResultDump)
+	testServer.Response(404, nil, NoSuchUploadErrorDump)
+	testServer.Response(200, etag1, "")
+
+	b := s.s3.Bucket("sample")
+
+	multi, err := b.InitMulti("multi", "text/plain", s3.Private)
+	c.Assert(err, IsNil)
+
+	// Must send at least one part, so that completing it will work.
+	parts, err := multi.PutAll(strings.NewReader(""), 5)
+	c.Assert(parts, HasLen, 1)
+	c.Assert(parts[0].ETag, Equals, `"etag1"`)
+	c.Assert(err, IsNil)
+
+	// Init
+	testServer.WaitRequest()
+
+	// List old parts. Won't find anything.
+	req := testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "GET")
+	c.Assert(req.URL.Path, Equals, "/sample/multi")
+
+	// Send empty part.
+	req = testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "PUT")
+	c.Assert(req.URL.Path, Equals, "/sample/multi")
+	c.Assert(req.Form["partNumber"], DeepEquals, []string{"1"})
+	c.Assert(req.Header["Content-Length"], DeepEquals, []string{"0"})
+	c.Assert(readAll(req.Body), Equals, "")
+}
+
+func (s *S) TestPutAllResume(c *C) {
+	etag2 := map[string]string{"ETag": `"etag2"`}
 	testServer.Response(200, nil, InitMultiResultDump)
 	testServer.Response(200, nil, ListPartsResultDump1)
-	testServer.Response(404, nil, NoSuchUploadErrorDump) // :-(
 	testServer.Response(200, nil, ListPartsResultDump2)
+	testServer.Response(200, etag2, "")
 
 	b := s.s3.Bucket("sample")
 
 	multi, err := b.InitMulti("multi", "text/plain", s3.Private)
 	c.Assert(err, IsNil)
 
-	parts, err := multi.ListParts()
-	c.Assert(err, IsNil)
+	// "part1" and "part3" match the checksums in ResultDump1.
+	// The middle one is a mismatch (it refers to "part2").
+	parts, err := multi.PutAll(strings.NewReader("part1partXpart3"), 5)
 	c.Assert(parts, HasLen, 3)
 	c.Assert(parts[0].N, Equals, 1)
-	c.Assert(parts[0].Size, Equals, int64(8))
-	c.Assert(parts[0].ETag, Equals, `"26f90efd10d614f100252ff56d88dad8"`)
+	c.Assert(parts[0].Size, Equals, int64(5))
+	c.Assert(parts[0].ETag, Equals, `"ffc88b4ca90a355f8ddba6b2c3b2af5c"`)
 	c.Assert(parts[1].N, Equals, 2)
-	c.Assert(parts[1].Size, Equals, int64(8))
-	c.Assert(parts[1].ETag, Equals, `"b572ef59bfa4a719fdf2b3c13c583af8"`)
+	c.Assert(parts[1].Size, Equals, int64(5))
+	c.Assert(parts[1].ETag, Equals, `"etag2"`)
 	c.Assert(parts[2].N, Equals, 3)
-	c.Assert(parts[2].Size, Equals, int64(4))
-	c.Assert(parts[2].ETag, Equals, `"50dbe110509c7952ba70a96587bd7c40"`)
+	c.Assert(parts[2].Size, Equals, int64(5))
+	c.Assert(parts[2].ETag, Equals, `"49dcd91231f801159e893fb5c6674985"`)
+	c.Assert(err, IsNil)
 
+	// Init
 	testServer.WaitRequest()
+
+	// List old parts, broken in two requests.
+	for i := 0; i < 2; i++ {
+		req := testServer.WaitRequest()
+		c.Assert(req.Method, Equals, "GET")
+		c.Assert(req.URL.Path, Equals, "/sample/multi")
+	}
+
+	// Send part 2, as it didn't match the checksum.
 	req := testServer.WaitRequest()
-	c.Assert(req.Method, Equals, "GET")
-	c.Assert(req.URL.Path, Equals, "/sample/multi")
-	c.Assert(req.Form.Get("uploadId"), Matches, "JNbR_[A-Za-z0-9.]+QQ--")
-	c.Assert(req.Form["max-parts"], DeepEquals, []string{"1000"})
-
-	testServer.WaitRequest() // The internal error.
-	req = testServer.WaitRequest()
-	c.Assert(req.Method, Equals, "GET")
-	c.Assert(req.URL.Path, Equals, "/sample/multi")
-	c.Assert(req.Form.Get("uploadId"), Matches, "JNbR_[A-Za-z0-9.]+QQ--")
-	c.Assert(req.Form["max-parts"], DeepEquals, []string{"1000"})
-	c.Assert(req.Form["part-number-marker"], DeepEquals, []string{"2"})
+	c.Assert(req.Method, Equals, "PUT")
+	c.Assert(req.URL.Path, Equals, "/sample/multi")
+	c.Assert(req.Form["partNumber"], DeepEquals, []string{"2"})
+	c.Assert(req.Header["Content-Length"], DeepEquals, []string{"5"})
+	c.Assert(readAll(req.Body), Equals, "partX")
 }
 
 func (s *S) TestMultiComplete(c *C) {
@@ -115,11 +307,11 @@
 	c.Assert(req.URL.Path, Equals, "/sample/multi")
 	c.Assert(req.Form.Get("uploadId"), Matches, "JNbR_[A-Za-z0-9.]+QQ--")
 
-	var payload struct{
+	var payload struct {
 		XMLName xml.Name
-		Part []struct{
+		Part    []struct {
 			PartNumber int
-			ETag string
+			ETag       string
 		}
 	}
 

=== modified file 's3/responses_test.go'
--- s3/responses_test.go	2013-02-01 05:58:42 +0000
+++ s3/responses_test.go	2013-02-13 12:22:21 +0000
@@ -87,14 +87,14 @@
   <Part>
     <PartNumber>1</PartNumber>
     <LastModified>2013-01-30T13:45:51.000Z</LastModified>
-    <ETag>&quot;26f90efd10d614f100252ff56d88dad8&quot;</ETag>
-    <Size>8</Size>
+    <ETag>&quot;ffc88b4ca90a355f8ddba6b2c3b2af5c&quot;</ETag>
+    <Size>5</Size>
   </Part>
   <Part>
     <PartNumber>2</PartNumber>
     <LastModified>2013-01-30T13:45:52.000Z</LastModified>
-    <ETag>&quot;b572ef59bfa4a719fdf2b3c13c583af8&quot;</ETag>
-    <Size>8</Size>
+    <ETag>&quot;d067a0fa9dc61a6e7195ca99696b5a89&quot;</ETag>
+    <Size>5</Size>
   </Part>
 </ListPartsResult>
 `
@@ -121,8 +121,8 @@
   <Part>
     <PartNumber>3</PartNumber>
     <LastModified>2013-01-30T13:46:50.000Z</LastModified>
-    <ETag>&quot;50dbe110509c7952ba70a96587bd7c40&quot;</ETag>
-    <Size>4</Size>
+    <ETag>&quot;49dcd91231f801159e893fb5c6674985&quot;</ETag>
+    <Size>5</Size>
   </Part>
 </ListPartsResult>
 `

=== modified file 's3/s3_test.go'
--- s3/s3_test.go	2013-02-01 20:20:47 +0000
+++ s3/s3_test.go	2013-02-13 12:22:21 +0000
@@ -29,6 +29,13 @@
 	testServer.Start()
 	auth := aws.Auth{"abc", "123"}
 	s.s3 = s3.New(auth, aws.Region{Name: "faux-region-1", S3Endpoint: testServer.URL})
+}
+
+func (s *S) TearDownSuite(c *C) {
+	s3.SetAttemptStrategy(nil)
+}
+
+func (s *S) SetUpTest(c *C) {
 	attempts := aws.AttemptStrategy{
 		Total: 300 * time.Millisecond,
 		Delay: 100 * time.Millisecond,
@@ -36,14 +43,14 @@
 	s3.SetAttemptStrategy(&attempts)
 }
 
-func (s *S) TearDownSuite(c *C) {
-	s3.SetAttemptStrategy(nil)
-}
-
 func (s *S) TearDownTest(c *C) {
 	testServer.Flush()
 }
 
+func (s *S) DisableRetries() {
+	s3.SetAttemptStrategy(&aws.AttemptStrategy{})
+}
+
 // PutBucket docs: http://goo.gl/kBTCu
 
 func (s *S) TestPutBucket(c *C) {

=== modified file 's3/s3i_test.go'
--- s3/s3i_test.go	2013-02-11 21:32:40 +0000
+++ s3/s3i_test.go	2013-02-13 12:22:21 +0000
@@ -102,7 +102,7 @@
 }
 
 var attempts = aws.AttemptStrategy{
-	Min: 5,
+	Min:   5,
 	Total: 20 * time.Second,
 	Delay: 100 * time.Millisecond,
 }
@@ -567,3 +567,24 @@
 	c.Assert(multis[1].Key, Equals, "a/multi3")
 	c.Assert(multis[1].UploadId, Matches, ".+")
 }
+
+func (s *ClientTests) TestMultiPutAllZeroLength(c *C) {
+	b := testBucket(s.s3)
+	err := b.PutBucket(s3.Private)
+	c.Assert(err, IsNil)
+
+	multi, err := b.InitMulti("multi", "text/plain", s3.Private)
+	c.Assert(err, IsNil)
+	defer multi.Abort()
+
+	// This tests an edge case. Amazon requires at least one
+	// part for multiprat uploads to work, even the part is empty.
+	parts, err := multi.PutAll(strings.NewReader(""), 5*1024*1024)
+	c.Assert(err, IsNil)
+	c.Assert(parts, HasLen, 1)
+	c.Assert(parts[0].Size, Equals, int64(0))
+	c.Assert(parts[0].ETag, Equals, `"d41d8cd98f00b204e9800998ecf8427e"`)
+
+	err = multi.Complete(parts)
+	c.Assert(err, IsNil)
+}