From f0894a00f4b756d4b9b4078af2e686b359493583 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Thu, 13 Jul 2023 16:40:49 +0200 Subject: [PATCH] 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 Reviewed-by: Alex Brainman Reviewed-by: Bryan Mills Run-TryBot: Quim Muntal Reviewed-by: Dmitri Shuralyov TryBot-Result: Gopher Robot --- src/os/exec_windows.go | 6 --- src/os/exec_windows_test.go | 83 +++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 6 deletions(-) create mode 100644 src/os/exec_windows_test.go diff --git a/src/os/exec_windows.go b/src/os/exec_windows.go index 239bed198f..061a12b10f 100644 --- a/src/os/exec_windows.go +++ b/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 } diff --git a/src/os/exec_windows_test.go b/src/os/exec_windows_test.go new file mode 100644 index 0000000000..f8ed4cdf1c --- /dev/null +++ b/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() +} -- 2.48.1