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>

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.
+	// Remove contents & return first error.
+	err = nil
+	for {
 		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.
+				// Already deleted by someone else.
 				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)
-		}
-		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
 		}
+
+		// 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
 			}
 
-	// Close directory, because windows won't remove opened directory.
-	fd.Close()
+			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)