os: remove 5ms sleep on Windows in (*Process).Wait

The 5ms sleep in (*Process).Wait was added to mitigate errors while
removing executable files using os.RemoveAll.

Windows 10 1903 implements POSIX semantics for DeleteFile, making the
implementation of os.RemoveAll on Windows much more robust. Older
Windows 10 versions also made internal improvements to avoid errors
when removing files, making it less likely that the 5ms sleep is
necessary.

Windows 10 is the oldest version that Go supports (see #57004), so it
makes sense to unconditionally remove the 5ms sleep now. We have all
the Go 1.22 development cycle to see if this causes any regression.

Fixes #25965

Change-Id: Ie0bbe6dc3e8389fd51a32484d5d20cf59b019451
Reviewed-on: https://go-review.googlesource.com/c/go/+/509335
Reviewed-by: Brad Fitzpatrick <[email protected]>
Reviewed-by: Alex Brainman <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
Run-TryBot: Quim Muntal <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: qmuntal

src/os/exec_windows.go

@@ -35,12 +35,6 @@ func (p *Process) wait() (ps *ProcessState, err error) {
 		return nil, NewSyscallError("GetProcessTimes", e)
 	}
 	p.setDone()
-	// NOTE(brainman): It seems that sometimes process is not dead
-	// when WaitForSingleObject returns. But we do not know any
-	// other way to wait for it. Sleeping for a while seems to do
-	// the trick sometimes.
-	// See https://golang.org/issue/25965 for details.
-	defer time.Sleep(5 * time.Millisecond)
 	defer p.Release()
 	return &ProcessState{p.Pid, syscall.WaitStatus{ExitCode: ec}, &u}, nil
 }

src/os/exec_windows_test.go

@@ -0,0 +1,83 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build windows
+
+package os_test
+
+import (
+	"internal/testenv"
+	"io"
+	. "os"
+	"path/filepath"
+	"sync"
+	"testing"
+)
+
+func TestRemoveAllWithExecutedProcess(t *testing.T) {
+	// Regression test for golang.org/issue/25965.
+	if testing.Short() {
+		t.Skip("slow test; skipping")
+	}
+	testenv.MustHaveExec(t)
+
+	name, err := Executable()
+	if err != nil {
+		t.Fatal(err)
+	}
+	r, err := Open(name)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer r.Close()
+	const n = 100
+	var execs [n]string
+	// First create n executables.
+	for i := 0; i < n; i++ {
+		// Rewind r.
+		if _, err := r.Seek(0, io.SeekStart); err != nil {
+			t.Fatal(err)
+		}
+		name := filepath.Join(t.TempDir(), "test.exe")
+		execs[i] = name
+		w, err := Create(name)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if _, err = io.Copy(w, r); err != nil {
+			w.Close()
+			t.Fatal(err)
+		}
+		if err := w.Sync(); err != nil {
+			w.Close()
+			t.Fatal(err)
+		}
+		if err = w.Close(); err != nil {
+			t.Fatal(err)
+		}
+	}
+	// Then run each executable and remove its directory.
+	// Run each executable in a separate goroutine to add some load
+	// and increase the chance of triggering the bug.
+	var wg sync.WaitGroup
+	wg.Add(n)
+	for i := 0; i < n; i++ {
+		go func(i int) {
+			defer wg.Done()
+			name := execs[i]
+			dir := filepath.Dir(name)
+			// Run test.exe without executing any test, just to make it do something.
+			cmd := testenv.Command(t, name, "-test.run=^$")
+			if err := cmd.Run(); err != nil {
+				t.Errorf("exec failed: %v", err)
+			}
+			// Remove dir and check that it doesn't return `ERROR_ACCESS_DENIED`.
+			err = RemoveAll(dir)
+			if err != nil {
+				t.Errorf("RemoveAll failed: %v", err)
+			}
+		}(i)
+	}
+	wg.Wait()
+}