From 441951a93b0d3c6b4ac1be1e8731634d4b3d737d Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Fri, 4 Nov 2016 17:06:56 +0000 Subject: [PATCH] Stop removing failed upload to cloud storage remotes - fixes #559 We do remove a partially written file on local so we don't have corrupted files lying around. --- amazonclouddrive/amazonclouddrive_test.go | 1 + b2/b2_test.go | 1 + crypt/crypt2_test.go | 1 + crypt/crypt_test.go | 1 + drive/drive_test.go | 1 + dropbox/dropbox_test.go | 1 + fs/operations.go | 6 ---- fstest/fstests/fstests.go | 32 +++++++++++++++++++ googlecloudstorage/googlecloudstorage_test.go | 1 + hubic/hubic_test.go | 1 + local/local.go | 14 +++++--- local/local_test.go | 1 + onedrive/onedrive_test.go | 1 + s3/s3_test.go | 1 + swift/swift_test.go | 1 + yandex/yandex_test.go | 1 + 16 files changed, 54 insertions(+), 11 deletions(-) diff --git a/amazonclouddrive/amazonclouddrive_test.go b/amazonclouddrive/amazonclouddrive_test.go index 44dfca0fc..0fc3d99f8 100644 --- a/amazonclouddrive/amazonclouddrive_test.go +++ b/amazonclouddrive/amazonclouddrive_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/b2/b2_test.go b/b2/b2_test.go index 519f99c55..c06f633df 100644 --- a/b2/b2_test.go +++ b/b2/b2_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/crypt/crypt2_test.go b/crypt/crypt2_test.go index ffd359b24..8a19292c0 100644 --- a/crypt/crypt2_test.go +++ b/crypt/crypt2_test.go @@ -28,6 +28,7 @@ func TestFsListEmpty2(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty2(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound2(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile12(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError2(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile22(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile12(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile22(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/crypt/crypt_test.go b/crypt/crypt_test.go index d7481f478..fbbef92fb 100644 --- a/crypt/crypt_test.go +++ b/crypt/crypt_test.go @@ -28,6 +28,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/drive/drive_test.go b/drive/drive_test.go index 1a92c3de8..0d3db28d0 100644 --- a/drive/drive_test.go +++ b/drive/drive_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/dropbox/dropbox_test.go b/dropbox/dropbox_test.go index e41ffa6f9..8f1c31391 100644 --- a/dropbox/dropbox_test.go +++ b/dropbox/dropbox_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/fs/operations.go b/fs/operations.go index 8ae3e7697..034ce21e4 100644 --- a/fs/operations.go +++ b/fs/operations.go @@ -265,11 +265,6 @@ func Copy(f Fs, dst, src Object) (err error) { // Retry if err returned a retry error if IsRetryError(err) || ShouldRetry(err) { Debug(src, "Received error: %v - low level retry %d/%d", err, tries, maxTries) - if removeFailedCopy(dst) { - // If we removed dst, then nil it out and note we are not updating - dst = nil - doUpdate = false - } continue } // otherwise finish @@ -278,7 +273,6 @@ func Copy(f Fs, dst, src Object) (err error) { if err != nil { Stats.Error() ErrorLog(src, "Failed to copy: %v", err) - removeFailedCopy(dst) return err } diff --git a/fstest/fstests/fstests.go b/fstest/fstests/fstests.go index 2d7626e28..62699a4fd 100644 --- a/fstest/fstests/fstests.go +++ b/fstest/fstests/fstests.go @@ -7,6 +7,7 @@ package fstests import ( "bytes" + "errors" "flag" "fmt" "io" @@ -235,6 +236,37 @@ func TestFsPutFile1(t *testing.T) { file1Contents = testPut(t, &file1) } +type errorReader struct { + err error +} + +func (er errorReader) Read(p []byte) (n int, err error) { + return 0, er.err +} + +// TestFsPutError tests uploading a file where there is an error +// +// It makes sure that aborting a file half way through does not create +// a file on the remote. +func TestFsPutError(t *testing.T) { + skipIfNotOk(t) + + // Read 50 bytes then produce an error + contents := fstest.RandomString(50) + buf := bytes.NewBufferString(contents) + er := &errorReader{errors.New("potato")} + in := io.MultiReader(buf, er) + + obji := fs.NewStaticObjectInfo(file2.Path, file2.ModTime, 100, true, nil, nil) + obj, err := remote.Put(in, obji) + // assert.Nil(t, obj) - FIXME some remotes return the object even on nil + assert.NotNil(t, err) + + obj, err = remote.NewObject(file2.Path) + assert.Nil(t, obj) + assert.Equal(t, fs.ErrorObjectNotFound, err) +} + // TestFsPutFile2 tests putting a file into a subdirectory func TestFsPutFile2(t *testing.T) { skipIfNotOk(t) diff --git a/googlecloudstorage/googlecloudstorage_test.go b/googlecloudstorage/googlecloudstorage_test.go index 24147e3a1..ef3431185 100644 --- a/googlecloudstorage/googlecloudstorage_test.go +++ b/googlecloudstorage/googlecloudstorage_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/hubic/hubic_test.go b/hubic/hubic_test.go index 124bccb67..0788a207f 100644 --- a/hubic/hubic_test.go +++ b/hubic/hubic_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/local/local.go b/local/local.go index 1974f1044..375952ce4 100644 --- a/local/local.go +++ b/local/local.go @@ -665,12 +665,16 @@ func (o *Object) Update(in io.Reader, src fs.ObjectInfo) error { in = io.TeeReader(in, hash) _, err = io.Copy(out, in) - outErr := out.Close() - if err != nil { - return err + closeErr := out.Close() + if err == nil { + err = closeErr } - if outErr != nil { - return outErr + if err != nil { + fs.Debug(o, "Removing partially written file on error: %v", err) + if removeErr := os.Remove(o.path); removeErr != nil { + fs.ErrorLog(o, "Failed to remove partially written file: %v", removeErr) + } + return err } // All successful so update the hashes diff --git a/local/local_test.go b/local/local_test.go index eb9f91a57..f197605ce 100644 --- a/local/local_test.go +++ b/local/local_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/onedrive/onedrive_test.go b/onedrive/onedrive_test.go index 2fdcab36d..01eeafb47 100644 --- a/onedrive/onedrive_test.go +++ b/onedrive/onedrive_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/s3/s3_test.go b/s3/s3_test.go index fdb646fd8..1ec896515 100644 --- a/s3/s3_test.go +++ b/s3/s3_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/swift/swift_test.go b/swift/swift_test.go index 27d78b45c..d5a7edde9 100644 --- a/swift/swift_test.go +++ b/swift/swift_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) } diff --git a/yandex/yandex_test.go b/yandex/yandex_test.go index cd35dc043..5f7eba444 100644 --- a/yandex/yandex_test.go +++ b/yandex/yandex_test.go @@ -27,6 +27,7 @@ func TestFsListEmpty(t *testing.T) { fstests.TestFsListEmpty(t) } func TestFsListDirEmpty(t *testing.T) { fstests.TestFsListDirEmpty(t) } func TestFsNewObjectNotFound(t *testing.T) { fstests.TestFsNewObjectNotFound(t) } func TestFsPutFile1(t *testing.T) { fstests.TestFsPutFile1(t) } +func TestFsPutError(t *testing.T) { fstests.TestFsPutError(t) } func TestFsPutFile2(t *testing.T) { fstests.TestFsPutFile2(t) } func TestFsUpdateFile1(t *testing.T) { fstests.TestFsUpdateFile1(t) } func TestFsListDirFile2(t *testing.T) { fstests.TestFsListDirFile2(t) }