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

runtime: false positive deadlock when using testing.T.Fatal from goroutine #20908

Closed
abergmeier-dsfishlabs opened this issue Jul 5, 2017 · 7 comments

Comments

Projects
None yet
6 participants
@abergmeier-dsfishlabs
Copy link

commented Jul 5, 2017

What version of Go are you using (go version)?

1.8.3

What operating system and processor architecture are you using (go env)?

Ubuntu 16.04.2

What did you do?

func Test(t *testing.T) {
    t.Fatalf("Something went wrong")
}

go func() {
    go func() {
        Test(t)
        done <- true
    }
}()

<- done

What did you expect to see?

Anything but a false positive with a misleading error message.

What did you see instead?

fatal error: all goroutines are asleep - deadlock!
@cznic

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2017

There's no way to prove it's a false positive without full compilable code. Please provide it.

Anyway, from guessing what the code may look like, I think it's a quite real deadlock.

@abergmeier-dsfishlabs

This comment has been minimized.

Copy link
Author

commented Jul 5, 2017

I think it's a quite real deadlock.

It is not. Reading the docs of t.FailNow, this is an API constraint (only call FailNow from testing goroutine) that the detector does not sort out.
My problem here is that the error message is misleading and people will need hours to figure out that it is not an "actual concurrency problem" of their code.

IMO it should work like this: testing.T.FailNow should set a flag aboutToAbortAndAmNotADeadlock, which the deadlock detector then checks and knows it does not need to run.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2017

It is not.

Show the code. Nothing else matters.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 5, 2017

I think you've misinterpreted the FailNow docs. It ends the goroutine (via calling runtime.Goexit), so if there is stuff happening after that point, it won't run. And if the stuff happening after that is what causes your program to NOT deadlock, then you certainly might have a deadlock. It's not possible for the testing package to guarantee the non-existence of bugs elsewhere in your program.

@odeke-em odeke-em changed the title reports a false positive deadlock when using testing.T.Fatal from goroutine runtime: false positive deadlock when using testing.T.Fatal from goroutine Jul 10, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Jul 10, 2017

I've tweaked the bug's title a bit and I'll also page some runtime and deadlock folks @aclements @dvyukov, s'il vous plait.

@aclements

This comment has been minimized.

Copy link
Member

commented Jul 10, 2017

It is not. Reading the docs of t.FailNow, this is an API constraint (only call FailNow from testing goroutine) that the detector does not sort out.

It's not the deadlock detector's responsibility to enforce API constraints. Maybe FailNow should enforce that it's only called from a testing goroutine and give a more useful error if it is not.

My problem here is that the error message is misleading and people will need hours to figure out that it is not an "actual concurrency problem" of their code.

It's obviously hard to say without a more complete code example, but this looks like it is an actual concurrency problem and a very real deadlock. I would expect the traceback to point at the <-done line, which should make you ask why the done <- true never executed. Is this not what happened?

Separately, it's an interesting question whether a deadlock should kill the entire test run or just result in a test failure. But the latter would require a fair amount of new runtime API surface that doesn't seem worth its weight. And it would still report it as a deadlock; it would just keep going with the other tests.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 5, 2017

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@gopherbot gopherbot closed this Aug 5, 2017

@golang golang locked and limited conversation to collaborators Aug 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.