Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: "panic: Fail in goroutine after TestKillStartProcess has completed" #43722

Closed
zx2c4 opened this issue Jan 15, 2021 · 2 comments
Closed

os: "panic: Fail in goroutine after TestKillStartProcess has completed" #43722

zx2c4 opened this issue Jan 15, 2021 · 2 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Jan 15, 2021

Another flaky test:

=== RUN   TestKillFindProcess
panic: Fail in goroutine after TestKillStartProcess has completed

goroutine 7417 [running]:
testing.(*common).Fail(0xc0007e4600)
        C:/Go/src/testing/testing.go:698 +0x127
testing.(*common).FailNow(0xc0007e4600)
        C:/Go/src/testing/testing.go:720 +0x32
testing.(*common).Fatalf(0xc0007e4600, 0xa47d8a, 0x1f, 0xc0006d9f98, 0x1, 0x1)
        C:/Go/src/testing/testing.go:816 +0x97
os_test.TestKillStartProcess.func1(0xc0003ce8d0)
        C:/Go/src/os/os_test.go:2331 +0xc7
os_test.testKillProcess.func1(0xc00028a0d0, 0xc000298000)
        C:/Go/src/os/os_test.go:2310 +0x4b
created by os_test.testKillProcess
        C:/Go/src/os/os_test.go:2308 +0x157
FAIL    os      67.523s
FAIL
go: failed to remove work dir: remove C:\Go\tmp\go-build3763818576\b001\os.test.exe: Access is denied.

cc @bcmills

go test -v  os -count 10000
@bcmills bcmills changed the title windows: panic: Fail in goroutine after TestKillStartProcess has completed os: "panic: Fail in goroutine after TestKillStartProcess has completed" on Windows Jan 15, 2021
@bcmills bcmills added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure. labels Jan 15, 2021
@bcmills bcmills added this to the Backlog milestone Jan 15, 2021
@bcmills bcmills changed the title os: "panic: Fail in goroutine after TestKillStartProcess has completed" on Windows os: "panic: Fail in goroutine after TestKillStartProcess has completed" Jan 15, 2021
@bcmills
Copy link
Contributor

bcmills commented Jan 15, 2021

This turns out not to be Windows-specific. There are at least two defects in the test.

  1. The test does not wait for the processKiller callback to return, so it may fail after the test function has already returned (as observed here).

    go/src/os/os_test.go

    Lines 2308 to 2311 in 0c5afc4

    go func() {
    time.Sleep(100 * time.Millisecond)
    processKiller(cmd.Process)
    }()

  2. The callback invokes t.Fatalf from a non-main goroutine — which may invoke runtime.Goexit — but does not propagate the Goexit back to the caller. It should probably use t.Errorf instead.

    go/src/os/os_test.go

    Lines 2328 to 2333 in 0c5afc4

    testKillProcess(t, func(p *Process) {
    err := p.Kill()
    if err != nil {
    t.Fatalf("Failed to kill test process: %v", err)
    }
    })

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 15, 2021
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 15, 2021
@gopherbot
Copy link

Change https://golang.org/cl/284153 mentions this issue: os: invoke processKiller synchronously in testKillProcess

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

3 participants