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>
var Atime = atime
var LstatP = &lstat
var ErrWriteAtInAppendMode = errWriteAtInAppendMode
+var RemoveAllTestHook = &removeAllTestHook
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
// 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)
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
+ for {
+ numErr := 0
+
+ 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
+ }
+ return &PathError{"readdirnames", base, readErr}
}
- return &PathError{"readdirnames", base, readErr}
- }
- for _, name := range names {
- err := removeAllFrom(file, name)
- if err != nil {
- if pathErr, ok := err.(*PathError); ok {
- pathErr.Path = base + string(PathSeparator) + pathErr.Path
+ 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
+ }
}
- 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
}
}
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
}
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
// 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
// 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
// Remove directory.
err1 := Remove(path)
+ err1 = removeAllTestHook(err1)
if err1 == nil || IsNotExist(err1) {
return nil
}
package os_test
import (
+ "errors"
"fmt"
"io/ioutil"
. "os"
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")
+ }
+}