fs: drop Stat() call from fsDeleteFile,deleteFile (#4744)

This commit makes fsDeleteFile() simply call deleteFile() after calling
the relevant path length checking functions. This DRYs the code base.

This commit removes the Stat() call from deleteFile(). This improves
performance and removes any possibility of a race condition.

This additionally adds tests and a benchmark for said function. The
results aren't very consistent, although I'd expect this commit to make
it faster.

cmd/fs-v1-helpers.go

@@ -363,8 +363,7 @@ func fsRenameFile(sourcePath, destPath string) error {
 	return nil
 }
 
-// Delete a file and its parent if it is empty at the destination path.
+// fsDeleteFile is a wrapper for deleteFile(), after checking the path length.
-// this function additionally protects the basePath from being deleted.
 func fsDeleteFile(basePath, deletePath string) error {
 	if err := checkPathLength(basePath); err != nil {
 		return traceError(err)
@@ -374,44 +373,7 @@ func fsDeleteFile(basePath, deletePath string) error {
 		return traceError(err)
 	}
 
-	if basePath == deletePath {
+	return deleteFile(basePath, deletePath)
-		return nil
-	}
-
-	// Verify if the path exists.
-	pathSt, err := osStat(preparePath(deletePath))
-	if err != nil {
-		if os.IsNotExist(err) {
-			return traceError(errFileNotFound)
-		} else if os.IsPermission(err) {
-			return traceError(errFileAccessDenied)
-		}
-		return traceError(err)
-	}
-
-	if pathSt.IsDir() && !isDirEmpty(deletePath) {
-		// Verify if directory is empty.
-		return nil
-	}
-
-	// Attempt to remove path.
-	if err = os.Remove(preparePath(deletePath)); err != nil {
-		if os.IsNotExist(err) {
-			return traceError(errFileNotFound)
-		} else if os.IsPermission(err) {
-			return traceError(errFileAccessDenied)
-		} else if isSysErrNotEmpty(err) {
-			return traceError(errVolumeNotEmpty)
-		}
-		return traceError(err)
-	}
-
-	// Recursively go down the next path and delete again.
-	if err := fsDeleteFile(basePath, pathutil.Dir(deletePath)); err != nil {
-		return err
-	}
-
-	return nil
 }
 
 // fsRemoveMeta safely removes a locked file and takes care of Windows special case

cmd/fs-v1-helpers_test.go

@@ -18,6 +18,7 @@ package cmd
 
 import (
 	"bytes"
+	"io"
 	"io/ioutil"
 	"os"
 	"path"
@@ -263,50 +264,123 @@ func TestFSDeletes(t *testing.T) {
 		t.Fatalf("Unable to create file, %s", err)
 	}
 	// Seek back.
-	reader.Seek(0, 0)
+	reader.Seek(0, io.SeekStart)
+
+	// folder is not empty
+	err = fsMkdir(pathJoin(path, "success-vol", "not-empty"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = ioutil.WriteFile(pathJoin(path, "success-vol", "not-empty", "file"), []byte("data"), 0777)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// recursive
+	if err = fsMkdir(pathJoin(path, "success-vol", "parent")); err != nil {
+		t.Fatal(err)
+	}
+	if err = fsMkdir(pathJoin(path, "success-vol", "parent", "dir")); err != nil {
+		t.Fatal(err)
+	}
 
 	testCases := []struct {
+		basePath    string
 		srcVol      string
 		srcPath     string
 		expectedErr error
 	}{
-		// Test case - 1.
 		// valid case with existing volume and file to delete.
 		{
+			basePath:    path,
 			srcVol:      "success-vol",
 			srcPath:     "success-file",
 			expectedErr: nil,
 		},
-		// Test case - 2.
 		// The file was deleted in the last case, so DeleteFile should fail.
 		{
+			basePath:    path,
 			srcVol:      "success-vol",
 			srcPath:     "success-file",
 			expectedErr: errFileNotFound,
 		},
-		// Test case - 3.
 		// Test case with segment of the volume name > 255.
 		{
+			basePath:    path,
 			srcVol:      "my-obj-del-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001",
 			srcPath:     "success-file",
 			expectedErr: errFileNameTooLong,
 		},
-		// Test case - 4.
 		// Test case with src path segment > 255.
 		{
+			basePath:    path,
 			srcVol:      "success-vol",
 			srcPath:     "my-obj-del-0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001",
 			expectedErr: errFileNameTooLong,
 		},
+		// Base path is way too long.
+		{
+			basePath:    "path03333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333",
+			srcVol:      "success-vol",
+			srcPath:     "object",
+			expectedErr: errFileNameTooLong,
+		},
+		// Directory is not empty. Should give nil, but won't delete.
+		{
+			basePath:    path,
+			srcVol:      "success-vol",
+			srcPath:     "not-empty",
+			expectedErr: nil,
+		},
+		// Should delete recursively.
+		{
+			basePath:    path,
+			srcVol:      "success-vol",
+			srcPath:     pathJoin("parent", "dir"),
+			expectedErr: nil,
+		},
 	}
 
 	for i, testCase := range testCases {
-		if err = fsDeleteFile(path, pathJoin(path, testCase.srcVol, testCase.srcPath)); errorCause(err) != testCase.expectedErr {
+		if err = fsDeleteFile(testCase.basePath, pathJoin(testCase.basePath, testCase.srcVol, testCase.srcPath)); errorCause(err) != testCase.expectedErr {
 			t.Errorf("Test case %d: Expected: \"%s\", got: \"%s\"", i+1, testCase.expectedErr, err)
 		}
 	}
 }
 
+func BenchmarkFSDeleteFile(b *testing.B) {
+	// create posix test setup
+	_, path, err := newPosixTestSetup()
+	if err != nil {
+		b.Fatalf("Unable to create posix test setup, %s", err)
+	}
+	defer removeAll(path)
+
+	// Setup test environment.
+	if err = fsMkdir(pathJoin(path, "benchmark")); err != nil {
+		b.Fatalf("Unable to create directory, %s", err)
+	}
+
+	benchDir := pathJoin(path, "benchmark")
+	filename := pathJoin(benchDir, "file.txt")
+
+	b.ResetTimer()
+	// We need to create and delete the file sequentially inside the benchmark.
+	for i := 0; i < b.N; i++ {
+		b.StopTimer()
+		err = ioutil.WriteFile(filename, []byte("data"), 0777)
+		if err != nil {
+			b.Fatal(err)
+		}
+		b.StartTimer()
+
+		err = fsDeleteFile(benchDir, filename)
+		if err != nil {
+			b.Fatal(err)
+		}
+	}
+}
+
 // Tests fs removes.
 func TestFSRemoves(t *testing.T) {
 	// create posix test setup

cmd/posix.go

@@ -915,27 +915,23 @@ func (s *posix) StatFile(volume, path string) (file FileInfo, err error) {
 	}, nil
 }
 
-// deleteFile - delete file path if its empty.
+// deleteFile deletes a file path if its empty. If it's successfully deleted,
+// it will recursively move up the tree, deleting empty parent directories
+// until it finds one with files in it. Returns nil for a non-empty directory.
 func deleteFile(basePath, deletePath string) error {
 	if basePath == deletePath {
 		return nil
 	}
-	// Verify if the path exists.
+
-	pathSt, err := osStat(preparePath(deletePath))
-	if err != nil {
-		if os.IsNotExist(err) {
-			return errFileNotFound
-		} else if os.IsPermission(err) {
-			return errFileAccessDenied
-		}
-		return err
-	}
-	if pathSt.IsDir() && !isDirEmpty(deletePath) {
-		// Verify if directory is empty.
-		return nil
-	}
 	// Attempt to remove path.
 	if err := os.Remove(preparePath(deletePath)); err != nil {
+		// Ignore errors if the directory is not empty. The server relies on
+		// this functionality, and sometimes uses recursion that should not
+		// error on parent directories.
+		if isSysErrNotEmpty(err) {
+			return nil
+		}
+
 		if os.IsNotExist(err) {
 			return errFileNotFound
 		} else if os.IsPermission(err) {
@@ -943,11 +939,9 @@ func deleteFile(basePath, deletePath string) error {
 		}
 		return err
 	}
+
 	// Recursively go down the next path and delete again.
-	if err := deleteFile(basePath, slashpath.Dir(deletePath)); err != nil {
+	return deleteFile(basePath, slashpath.Dir(deletePath))
-		return err
-	}
-	return nil
 }
 
 // DeleteFile - delete a file at path.