fs: Remove fs meta lock when PutObject() fails (#4114)

Removing the fs meta lock file when PutObject() encounters any error
during its execution, such as upload getting permatuerly cancelled
by the client.

cmd/fs-v1-helpers.go

@@ -20,6 +20,7 @@ import (
 	"io"
 	"os"
 	pathutil "path"
+	"runtime"
 )
 
 // Removes only the file at given path does not remove
@@ -377,3 +378,49 @@ func fsDeleteFile(basePath, deletePath string) error {
 
 	return nil
 }
+
+// fsRemoveMeta safely removes a locked file and takes care of Windows special case
+func fsRemoveMeta(basePath, deletePath, tmpDir string) error {
+	// Special case for windows please read through.
+	if runtime.GOOS == globalWindowsOSName {
+		// Ordinarily windows does not permit deletion or renaming of files still
+		// in use, but if all open handles to that file were opened with FILE_SHARE_DELETE
+		// then it can permit renames and deletions of open files.
+		//
+		// There are however some gotchas with this, and it is worth listing them here.
+		// Firstly, Windows never allows you to really delete an open file, rather it is
+		// flagged as delete pending and its entry in its directory remains visible
+		// (though no new file handles may be opened to it) and when the very last
+		// open handle to the file in the system is closed, only then is it truly
+		// deleted. Well, actually only sort of truly deleted, because Windows only
+		// appears to remove the file entry from the directory, but in fact that
+		// entry is merely hidden and actually still exists and attempting to create
+		// a file with the same name will return an access denied error. How long it
+		// silently exists for depends on a range of factors, but put it this way:
+		// if your code loops creating and deleting the same file name as you might
+		// when operating a lock file, you're going to see lots of random spurious
+		// access denied errors and truly dismal lock file performance compared to POSIX.
+		//
+		// We work-around these un-POSIX file semantics by taking a dual step to
+		// deleting files. Firstly, it renames the file to tmp location into multipartTmpBucket
+		// We always open files with FILE_SHARE_DELETE permission enabled, with that
+		// flag Windows permits renaming and deletion, and because the name was changed
+		// to a very random name somewhere not in its origin directory before deletion,
+		// you don't see those unexpected random errors when creating files with the
+		// same name as a recently deleted file as you do anywhere else on Windows.
+		// Because the file is probably not in its original containing directory any more,
+		// deletions of that directory will not fail with "directory not empty" as they
+		// otherwise normally would either.
+
+		tmpPath := pathJoin(tmpDir, mustGetUUID())
+
+		fsRenameFile(deletePath, tmpPath)
+
+		// Proceed to deleting the directory if empty
+		fsDeleteFile(basePath, pathutil.Dir(deletePath))
+
+		// Finally delete the renamed file.
+		return fsDeleteFile(tmpDir, tmpPath)
+	}
+	return fsDeleteFile(basePath, deletePath)
+}

cmd/fs-v1-helpers_test.go

@@ -18,7 +18,12 @@ package cmd
 
 import (
 	"bytes"
+	"io/ioutil"
+	"os"
+	"path"
 	"testing"
+
+	"github.com/minio/minio/pkg/lock"
 )
 
 func TestFSStats(t *testing.T) {
@@ -396,3 +401,52 @@ func TestFSRemoves(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestFSRemoveMeta(t *testing.T) {
+	// create posix test setup
+	_, fsPath, err := newPosixTestSetup()
+	if err != nil {
+		t.Fatalf("Unable to create posix test setup, %s", err)
+	}
+	defer removeAll(fsPath)
+
+	// Setup test environment.
+	if err = fsMkdir(pathJoin(fsPath, "success-vol")); err != nil {
+		t.Fatalf("Unable to create directory, %s", err)
+	}
+
+	filePath := pathJoin(fsPath, "success-vol", "success-file")
+
+	var buf = make([]byte, 4096)
+	var reader = bytes.NewReader([]byte("Hello, world"))
+	if _, err = fsCreateFile(filePath, reader, buf, reader.Size()); err != nil {
+		t.Fatalf("Unable to create file, %s", err)
+	}
+
+	rwPool := &fsIOPool{
+		readersMap: make(map[string]*lock.RLockedFile),
+	}
+
+	if _, err := rwPool.Open(filePath); err != nil {
+		t.Fatalf("Unable to lock file %s", filePath)
+	}
+
+	defer rwPool.Close(filePath)
+
+	tmpDir, tmpErr := ioutil.TempDir(globalTestTmpDir, "minio-")
+	if tmpErr != nil {
+		t.Fatal(tmpErr)
+	}
+
+	if err := fsRemoveMeta(fsPath, filePath, tmpDir); err != nil {
+		t.Fatalf("Unable to remove file, %s", err)
+	}
+
+	if _, err := os.Stat(filePath); !os.IsNotExist(err) {
+		t.Fatalf("`%s` file found though it should have been deleted.", filePath)
+	}
+
+	if _, err := os.Stat(path.Dir(filePath)); !os.IsNotExist(err) {
+		t.Fatalf("`%s` parent directory found though it should have been deleted.", filePath)
+	}
+}

cmd/fs-v1-multipart.go

@@ -24,7 +24,6 @@ import (
 	"io"
 	"os"
 	pathutil "path"
-	"runtime"
 	"strings"
 	"time"
 
@@ -46,56 +45,15 @@ func (fs fsObjects) isMultipartUpload(bucket, prefix string) bool {
 	return true
 }
 
-// Delete uploads.json file wrapper handling a tricky case on windows.
+// Delete uploads.json file wrapper
 func (fs fsObjects) deleteUploadsJSON(bucket, object, uploadID string) error {
-	timeID := fmt.Sprintf("%X", UTCNow().UnixNano())
-	tmpPath := pathJoin(fs.fsPath, minioMetaTmpBucket, fs.fsUUID, uploadID+"+"+timeID)
-
 	multipartBucketPath := pathJoin(fs.fsPath, minioMetaMultipartBucket)
 	uploadPath := pathJoin(multipartBucketPath, bucket, object)
 	uploadsMetaPath := pathJoin(uploadPath, uploadsJSONFile)
 
-	// Special case for windows please read through.
+	tmpDir := pathJoin(fs.fsPath, minioMetaTmpBucket, fs.fsUUID)
-	if runtime.GOOS == globalWindowsOSName {
-		// Ordinarily windows does not permit deletion or renaming of files still
-		// in use, but if all open handles to that file were opened with FILE_SHARE_DELETE
-		// then it can permit renames and deletions of open files.
-		//
-		// There are however some gotchas with this, and it is worth listing them here.
-		// Firstly, Windows never allows you to really delete an open file, rather it is
-		// flagged as delete pending and its entry in its directory remains visible
-		// (though no new file handles may be opened to it) and when the very last
-		// open handle to the file in the system is closed, only then is it truly
-		// deleted. Well, actually only sort of truly deleted, because Windows only
-		// appears to remove the file entry from the directory, but in fact that
-		// entry is merely hidden and actually still exists and attempting to create
-		// a file with the same name will return an access denied error. How long it
-		// silently exists for depends on a range of factors, but put it this way:
-		// if your code loops creating and deleting the same file name as you might
-		// when operating a lock file, you're going to see lots of random spurious
-		// access denied errors and truly dismal lock file performance compared to POSIX.
-		//
-		// We work-around these un-POSIX file semantics by taking a dual step to
-		// deleting files. Firstly, it renames the file to tmp location into multipartTmpBucket
-		// We always open files with FILE_SHARE_DELETE permission enabled, with that
-		// flag Windows permits renaming and deletion, and because the name was changed
-		// to a very random name somewhere not in its origin directory before deletion,
-		// you don't see those unexpected random errors when creating files with the
-		// same name as a recently deleted file as you do anywhere else on Windows.
-		// Because the file is probably not in its original containing directory any more,
-		// deletions of that directory will not fail with "directory not empty" as they
-		// otherwise normally would either.
-		fsRenameFile(uploadsMetaPath, tmpPath)
-
-		// Proceed to deleting the directory.
-		if err := fsDeleteFile(multipartBucketPath, uploadPath); err != nil {
-			return err
-		}
 
-		// Finally delete the renamed file.
+	return fsRemoveMeta(multipartBucketPath, uploadsMetaPath, tmpDir)
-		return fsDeleteFile(pathutil.Dir(tmpPath), tmpPath)
-	}
-	return fsDeleteFile(multipartBucketPath, uploadsMetaPath)
 }
 
 // Removes the uploadID, called either by CompleteMultipart of AbortMultipart. If the resuling uploads

cmd/fs-v1.go

@@ -492,7 +492,9 @@ func (fs fsObjects) GetObjectInfo(bucket, object string) (ObjectInfo, error) {
 // until EOF, writes data directly to configured filesystem path.
 // Additionally writes `fs.json` which carries the necessary metadata
 // for future object operations.
-func (fs fsObjects) PutObject(bucket string, object string, size int64, data io.Reader, metadata map[string]string, sha256sum string) (objInfo ObjectInfo, err error) {
+func (fs fsObjects) PutObject(bucket string, object string, size int64, data io.Reader, metadata map[string]string, sha256sum string) (objInfo ObjectInfo, retErr error) {
+	var err error
+
 	// This is a special case with size as '0' and object ends with
 	// a slash separator, we treat it like a valid operation and
 	// return success.
@@ -517,13 +519,21 @@ func (fs fsObjects) PutObject(bucket string, object string, size int64, data io.
 
 	var wlk *lock.LockedFile
 	if bucket != minioMetaBucket {
-		fsMetaPath := pathJoin(fs.fsPath, minioMetaBucket, bucketMetaPrefix, bucket, object, fsMetaJSONFile)
+		bucketMetaDir := pathJoin(fs.fsPath, minioMetaBucket, bucketMetaPrefix)
+		fsMetaPath := pathJoin(bucketMetaDir, bucket, object, fsMetaJSONFile)
 		wlk, err = fs.rwPool.Create(fsMetaPath)
 		if err != nil {
 			return ObjectInfo{}, toObjectErr(traceError(err), bucket, object)
 		}
 		// This close will allow for locks to be synchronized on `fs.json`.
 		defer wlk.Close()
+		defer func() {
+			// Remove meta file when PutObject encounters any error
+			if retErr != nil {
+				tmpDir := pathJoin(fs.fsPath, minioMetaTmpBucket, fs.fsUUID)
+				fsRemoveMeta(bucketMetaDir, fsMetaPath, tmpDir)
+			}
+		}()
 	}
 
 	// Uploaded object will first be written to the temporary location which will eventually