os: when looping in RemoveAll, close and re-open directory

On some systems removing files can cause a directory to be re-shuffled,
so simply continuing to read files can cause us to miss some.
Close and re-open the directory when looping, to avoid that.

Read more files each time through the loop, to reduce the chance of
having to re-open.

Fixes #20841

Change-Id: I98a14774ca63786ad05ba5000cbdb01ad2884332
Reviewed-on: https://go-review.googlesource.com/121255
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/os/path.go b/src/os/path.go
index 5c5350670d8cd4bb185289bc557827ef9be95afc..cdfbc189219a88d308c5eb82c79c49910ba6aec2 100644 (file)
--- a/src/os/path.go
+++ b/src/os/path.go
@@ -6,7 +6,6 @@ package os
 
 import (
        "io"
-       "runtime"
        "syscall"
 )
 
@@ -84,32 +83,35 @@ func RemoveAll(path string) error {
                return err
        }
 
-       // Directory.
-       fd, err := Open(path)
-       if err != nil {
-               if IsNotExist(err) {
-                       // Race. It was deleted between the Lstat and Open.
-                       // Return nil per RemoveAll's docs.
-                       return nil
-               }
-               return err
-       }
-
        // Remove contents & return first error.
        err = nil
        for {
-               if err == nil && (runtime.GOOS == "plan9" || runtime.GOOS == "nacl") {
-                       // Reset read offset after removing directory entries.
-                       // See golang.org/issue/22572.
-                       fd.Seek(0, 0)
+               fd, err := Open(path)
+               if err != nil {
+                       if IsNotExist(err) {
+                               // Already deleted by someone else.
+                               return nil
+                       }
+                       return err
                }
-               names, err1 := fd.Readdirnames(100)
+
+               const request = 1024
+               names, err1 := fd.Readdirnames(request)
+
+               // Removing files from the directory may have caused
+               // the OS to reshuffle it. Simply calling Readdirnames
+               // again may skip some entries. The only reliable way
+               // to avoid this is to close and re-open the
+               // 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 {
                        break
                }
@@ -120,10 +122,29 @@ func RemoveAll(path string) error {
                if len(names) == 0 {
                        break
                }
-       }
 
-       // Close directory, because windows won't remove opened directory.
-       fd.Close()
+               // We don't want to re-open unnecessarily, so if we
+               // got fewer than request names from Readdirnames, try
+               // simply removing the directory now. If that
+               // succeeds, we are done.
+               if len(names) < request {
+                       err1 := Remove(path)
+                       if err1 == nil || IsNotExist(err1) {
+                               return nil
+                       }
+
+                       if err != nil {
+                               // We got some error removing the
+                               // directory contents, and since we
+                               // read fewer names than we requested
+                               // there probably aren't more files to
+                               // remove. Don't loop around to read
+                               // the directory again. We'll probably
+                               // just get the same error.
+                               return err
+                       }
+               }
+       }
 
        // Remove directory.
        err1 := Remove(path)