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

testing: allow multiple async t.Fatals #25467

Closed
nhooyr opened this issue May 19, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@nhooyr
Copy link
Contributor

commented May 19, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version devel +e86c26789d Sat May 19 17:38:01 2018 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes but you encounter #24438 first so thats why I'm using a devel version.

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

macOS 10.13.4

What did you do?

package scratch

import (
	"sync"
	"testing"
)

func TestMeow(t *testing.T) {
	var wg sync.WaitGroup
	wg.Add(2)
	go func() {
		defer wg.Done()
		t.Fatal("meow")
	}()
	go func() {
		defer wg.Done()
		t.Fatal("meow")
	}()
	wg.Wait()
}
go test -race scratch_test.go

What did you expect to see?

Two test failures with error message "meow".

What did you see instead?

Race condition report due to the two concurrent writes at

c.finished = true
due to t.Fatal.

I think we should patch up this race. Its very similar to #24438 in that the test is racy, but not in an interesting way.

@agnivade agnivade changed the title allow multiple async t.Fatals testing: allow multiple async t.Fatals May 20, 2018

@agnivade agnivade added this to the Unplanned milestone May 20, 2018

@cznic

This comment has been minimized.

Copy link
Contributor

commented May 20, 2018

Is this a proposal or a bug report? I'm asking because this is working as documented.

A test ends when its Test function returns or calls any of the methods FailNow, Fatal, Fatalf, SkipNow, Skip, or Skipf. Those methods, as well as the Parallel method, must be called only from the goroutine running the Test function.

@mvdan

This comment has been minimized.

Copy link
Member

commented May 20, 2018

Like @cznic said, this issue is different from #24438. Error is documented to support being called by any goroutine, but Fatal is not.

However, perhaps the testing package could panic in a more obvious way, as opposed to silently working until someone uses the race detector.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2018

@cznic Its a proposal. I think it should be allowed. t.Fatal already works in another goroutine aside from main, so I think it should work with multiple goroutines as well.

@cznic

This comment has been minimized.

Copy link
Contributor

commented May 20, 2018

How do you propose to solve a situation where some TestFoo function launches two goroutines and one of them calls t.Fatal immediately while the other only [N minutes] later? The go test command may have even already finished before that could happen. Or worse, when the TestFoo function returns normally, but a goroutine launched by it calls/would call t.Fatal [N minutes] later?

IMO, the status quo is the only reasonable approach.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2018

How do you propose to solve a situation where some TestFoo function launches two goroutines and one of them calls t.Fatal immediately while the other only [N minutes] later? The go test command may have even already finished before that could happen.

The main test goroutine still needs to wait for the other two goroutines like it would with t.Error. So the go test command would not have finished before that could happen.

Or worse, when the TestFoo function returns normally, but a goroutine launched by it calls/would call t.Fatal [N minutes] later?

That should of course cause a panic like it does now.

@cznic

This comment has been minimized.

Copy link
Contributor

commented May 20, 2018

The main test goroutine still needs to wait for the other two goroutines like it would with t.Error. So the go test command would not have finished before that could happen.

How to do that? The goroutine that invokes TestMeow in the OP has no knowledge about it launching other goroutines that may possibly later call t.Fatal.

That should of course cause a panic like it does now.

This program does not panic now, nor it reports any error. The t.Fatal call is just silently lost because the program exits before calling time.After returned

func Test Foo(t *testing.T) {
        go func() {
                <-time.After(tooLong)
                t.Fatal("Vary dangerous error found.")
        }()
}
@nhooyr

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2018

How to do that? The goroutine that invokes TestMeow in the OP has no knowledge about it launching other goroutines that may possibly later call t.Fatal.

The user needs to sync by themselves. Its the same problem as with t.Error. Its not a distinct problem to t.Fatal.

This program does not panic now, nor it reports any error. The t.Fatal call is just silently lost because the program exits before calling time.After returned

The bug there is that you're not waiting for the main goroutine. The only way

func TestMeow(t *testing.T) {
	go func() {
		time.Sleep(time.Second)
		t.Fatal("Vary dangerous error found.")
	}()
	t.Log("done")
}

func TestBoo(t *testing.T) {
	time.Sleep(time.Second*3)
}

That however does produce the panic I was talking about. You can change the t.Fatal to t.Error to see it also occurs with t.Error.

@cznic

This comment has been minimized.

Copy link
Contributor

commented May 20, 2018

Its not a distinct problem to t.Fatal.

It's very different in that one terminates execution of the TestXXX function while the other does not. And that also explains why t.Error is allowed from any goroutine while t.Fatal is not.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2018

It's very different in that one terminates execution of the TestXXX function while the other does not. And that also explains why t.Error is allowed from any goroutine while t.Fatal is not.

Here's my argument. t.Error is allowed from any goroutine without any race conditions. t.Fatal is allowed from any goroutine too, even though it is documented that you should only be calling it from the main test goroutine. Go cannot enforce that t.Fatal should only be called from the main test goroutine. So at most, its a bug in that you are not following convention. If however, you run t.Fatal async in multiple goroutines, it goes from convention violation to race condition. I don't think that makes sense. Its not racey in an important way.

@bcmills

This comment has been minimized.

Copy link
Member

commented May 21, 2018

Go cannot enforce that t.Fatal should only be called from the main test goroutine.

That's not necessarily true. See #15758.

@mvdan

This comment has been minimized.

Copy link
Member

commented May 21, 2018

Thanks so much @bcmills - that is exactly what I was proposing in my comment :)

@nhooyr please refer to that issue as far as making the testing package's behavior more consistent and intuitive. If you would still like to propose that concurrent Fatal calls were supported and documented, feel free to open a new issue following the proposal process: https://github.com/golang/proposal

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