os: fix RemoveAll hangs on large directory

golang.org/cl/121255 added close and re-open the directory when looping, prevent
us from missing some if previous iteration deleted files.

The CL introdued a bug. If we can not delete all entries in one request,
the looping never exits, causing RemoveAll hangs.

To fix that, simply discard the entries if we can not delete all of them
in one iteration, then continue reading entries and delete them.

Also make sure removeall_at return first error it encounters.

Fixes #29921

Change-Id: I8ec3a4c822d8d2d95d9f1ab71547879da395bc4a
Reviewed-on: https://go-review.googlesource.com/c/go/+/171099
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

src/os/export_test.go

@@ -9,3 +9,4 @@ package os
 var Atime = atime
 var LstatP = &lstat
 var ErrWriteAtInAppendMode = errWriteAtInAppendMode
+var RemoveAllTestHook = &removeAllTestHook

src/os/path.go

@@ -58,6 +58,9 @@ func MkdirAll(path string, perm FileMode) error {
 	return nil
 }
 
+// removeAllTestHook is a hook for testing.
+var removeAllTestHook = func(err error) error { return err }
+
 // RemoveAll removes path and any children it contains.
 // It removes everything it can but returns the first error
 // it encounters. If the path does not exist, RemoveAll

src/os/removeall_at.go

@@ -91,7 +91,8 @@ func removeAllFrom(parent *File, base string) error {
 	// Remove the directory's entries.
 	var recurseErr error
 	for {
-		const request = 1024
+		const reqSize = 1024
+		var respSize int
 
 		// Open the directory to recurse into
 		file, err := openFdAt(parentFd, base)
@@ -103,23 +104,37 @@ func removeAllFrom(parent *File, base string) error {
 			break
 		}
 
-		names, readErr := file.Readdirnames(request)
-		// Errors other than EOF should stop us from continuing.
-		if readErr != nil && readErr != io.EOF {
-			file.Close()
-			if IsNotExist(readErr) {
-				return nil
-			}
-			return &PathError{"readdirnames", base, readErr}
-		}
+		for {
+			numErr := 0
 
-		for _, name := range names {
-			err := removeAllFrom(file, name)
-			if err != nil {
-				if pathErr, ok := err.(*PathError); ok {
-					pathErr.Path = base + string(PathSeparator) + pathErr.Path
+			names, readErr := file.Readdirnames(reqSize)
+			// Errors other than EOF should stop us from continuing.
+			if readErr != nil && readErr != io.EOF {
+				file.Close()
+				if IsNotExist(readErr) {
+					return nil
 				}
-				recurseErr = err
+				return &PathError{"readdirnames", base, readErr}
+			}
+
+			respSize = len(names)
+			for _, name := range names {
+				err := removeAllFrom(file, name)
+				if err != nil {
+					if pathErr, ok := err.(*PathError); ok {
+						pathErr.Path = base + string(PathSeparator) + pathErr.Path
+					}
+					numErr++
+					if recurseErr == nil {
+						recurseErr = err
+					}
+				}
+			}
+
+			// If we can delete any entry, break to start new iteration.
+			// Otherwise, we discard current names, get next entries and try deleting them.
+			if numErr != reqSize {
+				break
 			}
 		}
 
@@ -131,13 +146,14 @@ func removeAllFrom(parent *File, base string) error {
 		file.Close()
 
 		// Finish when the end of the directory is reached
-		if len(names) < request {
+		if respSize < reqSize {
 			break
 		}
 	}
 
 	// Remove the directory itself.
 	unlinkError := unix.Unlinkat(parentFd, base, unix.AT_REMOVEDIR)
+	unlinkError = removeAllTestHook(unlinkError)
 	if unlinkError == nil || IsNotExist(unlinkError) {
 		return nil
 	}

src/os/removeall_noat.go

@@ -56,8 +56,30 @@ func removeAll(path string) error {
 			return err
 		}
 
-		const request = 1024
-		names, err1 := fd.Readdirnames(request)
+		const reqSize = 1024
+		var names []string
+		var readErr error
+
+		for {
+			numErr := 0
+			names, readErr = fd.Readdirnames(reqSize)
+
+			for _, name := range names {
+				err1 := RemoveAll(path + string(PathSeparator) + name)
+				if err == nil {
+					err = err1
+				}
+				if err1 != nil {
+					numErr++
+				}
+			}
+
+			// If we can delete any entry, break to start new iteration.
+			// Otherwise, we discard current names, get next entries and try deleting them.
+			if numErr != reqSize {
+				break
+			}
+		}
 
 		// Removing files from the directory may have caused
 		// the OS to reshuffle it. Simply calling Readdirnames
@@ -66,19 +88,12 @@ func removeAll(path string) error {
 		// directory. See issue 20841.
 		fd.Close()
 
-		for _, name := range names {
-			err1 := RemoveAll(path + string(PathSeparator) + name)
-			if err == nil {
-				err = err1
-			}
-		}
-
-		if err1 == io.EOF {
+		if readErr == io.EOF {
 			break
 		}
 		// If Readdirnames returned an error, use it.
 		if err == nil {
-			err = err1
+			err = readErr
 		}
 		if len(names) == 0 {
 			break
@@ -88,7 +103,7 @@ func removeAll(path string) error {
 		// got fewer than request names from Readdirnames, try
 		// simply removing the directory now. If that
 		// succeeds, we are done.
-		if len(names) < request {
+		if len(names) < reqSize {
 			err1 := Remove(path)
 			if err1 == nil || IsNotExist(err1) {
 				return nil
@@ -109,6 +124,7 @@ func removeAll(path string) error {
 
 	// Remove directory.
 	err1 := Remove(path)
+	err1 = removeAllTestHook(err1)
 	if err1 == nil || IsNotExist(err1) {
 		return nil
 	}

src/os/removeall_test.go

@@ -5,6 +5,7 @@
 package os_test
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 	. "os"
@@ -405,3 +406,48 @@ func TestRemoveUnreadableDir(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+// Issue 29921
+func TestRemoveAllWithMoreErrorThanReqSize(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping in short mode")
+	}
+	oldRemoveAllTestHook := RemoveAllTestHook
+	*RemoveAllTestHook = func(err error) error {
+		return errors.New("error from RemoveAllTestHook")
+	}
+	defer func() {
+		*RemoveAllTestHook = *oldRemoveAllTestHook
+	}()
+
+	tmpDir, err := ioutil.TempDir("", "TestRemoveAll-")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer RemoveAll(tmpDir)
+
+	path := filepath.Join(tmpDir, "_TestRemoveAllWithMoreErrorThanReqSize_")
+
+	// Make directory with 1025 files and remove.
+	if err := MkdirAll(path, 0777); err != nil {
+		t.Fatalf("MkdirAll %q: %s", path, err)
+	}
+	for i := 0; i < 1025; i++ {
+		fpath := filepath.Join(path, fmt.Sprintf("file%d", i))
+		fd, err := Create(fpath)
+		if err != nil {
+			t.Fatalf("create %q: %s", fpath, err)
+		}
+		fd.Close()
+	}
+
+	// This call should not hang
+	if err := RemoveAll(path); err == nil {
+		t.Fatal("Want error from RemoveAllTestHook, got nil")
+	}
+
+	// We hook to inject error, but the actual files must be deleted
+	if _, err := Lstat(path); err == nil {
+		t.Fatal("directory must be deleted even with removeAllTetHook run")
+	}
+}