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

Jump to bottom

testing: complain loudly during concurrent use of T.FatalX and T.SkipX #15758

Open
dsnet opened this issue May 19, 2016 · 14 comments
Open

testing: complain loudly during concurrent use of T.FatalX and T.SkipX #15758

dsnet opened this issue May 19, 2016 · 14 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@dsnet
Copy link
Member

dsnet commented May 19, 2016 •
edited

The testing package is explicit about the following:

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.

However, this is easy to overlook and it is not immediately obvious that the code below is a bad test:

func TestFoo(t *testing.T) {
    go func() {
        t.Fatal("fatal") // Called from outside the Test function
    }()
    time.Sleep(1 * time.Second)
}

This currently outputs (what a user expects):

--- FAIL: TestFoo (1.00s)
    foo_test.go:10: fatal

However, since t.Fatal is not safe to call outside of the Test function, this is poor feedback since it makes the user think that the test is correctly written when it is actually racy. Instead it should complain loudly that this is wrong.

As I'm debugging poor tests, I've noticed that the calling of t.Fatal in a goroutine is actually a fairly common phenomenon.

Related: #15674

/cc @mpvl @bradfitz
@dsnet dsnet added this to the Go1.8 milestone May 19, 2016
@mpvl mpvl self-assigned this May 21, 2016
@dsnet dsnet mentioned this issue Oct 6, 2016
Closed
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016
@rsc
Copy link
Contributor

rsc commented Oct 18, 2016

Seems infeasible. FailNow/SkipNow are fundamentally built around runtime.Goexit, and there is no way to tell whether they are being called from the "right" goroutine. Bad API design maybe, but it's the way it is.

We can't change from runtime.Goexit to a panic in general; the whole point of using Goexit was that it does not look like a panic and cannot be stopped.

I don't see any way to change the current behavior nor do I see any way to make the current behavior louder.

@rsc rsc closed this as completed Oct 18, 2016
@bradfitz
Copy link
Contributor

@rsc, the http2 package has a testing-time-only mechanism to track that certain methods only run on the expected goroutines.

See https://github.com/golang/net/blob/master/http2/gotrack.go#L22

We could do something like that.

@rsc
Copy link
Contributor

rsc commented Oct 19, 2016

I've been wondering if we should take the goroutine ID out of the runtime.Stack output, precisely for this reason. :-(

@bradfitz
Copy link
Contributor

@rsc, that'd be fine, if you give me an as-useful debugging primitive elsewhere.

@golang golang locked and limited conversation to collaborators Oct 19, 2017
This was referenced Apr 4, 2018
Closed
Closed
@ianlancetaylor
Copy link
Contributor

Reopening as @balshetzer has an approach that may work, based on looking for certain functions in the stack trace. I guess we can give it a try and see if it seems reasonable.

@golang golang unlocked this conversation Apr 11, 2018
@dsnet dsnet modified the milestones: Go1.8, Go1.11 Apr 23, 2018
@odeke-em
Copy link
Member

For posterity, here is the permalink that @bradfitz referred to in his comment #15758 (comment)

https://github.com/golang/net/blob/2491c5de3490fced2f6cff376127c667efeed857/http2/gotrack.go#L22

@bcmills bcmills mentioned this issue May 21, 2018
Closed
@nhooyr
Copy link
Contributor

nhooyr commented Jun 27, 2018

As I'm debugging poor tests, I've noticed that the calling of t.Fatal in a goroutine is actually a fairly common phenomenon.

Another option here is to remove the doc and allow it. See #25467

@bcmills
Copy link
Contributor

bcmills commented Jun 27, 2018

@nhooyr, t.Fatal terminates the goroutine from which it is called (and runs any deferred function calls).

If we were to allow t.Fatal from any goroutine, how would the goroutine running the test function know to exit? It could be running any arbitrary function, so terminating it immediately would risk deadlocking future tests.

It is of course possible for the author of the test to plumb in some sort of cancellation or synchronization mechanism, but if they have done that, then they can just as easily invoke that mechanism instead of calling t.Fatal from the goroutine.

@nhooyr
Copy link
Contributor

nhooyr commented Jun 27, 2018

@bcmills We would change t.Fatal to report a error and exit whatever goroutine its called from. I don't think anyone would expect it to exit the main goroutine too.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 29, 2018
@gopherbot
Copy link

Change https://golang.org/cl/134395 mentions this issue: errgroup: rethink concurrency patterns

@bcmills
Copy link
Contributor

bcmills commented Sep 10, 2018

It turns out to be possible to propagate a runtime.Goexit from one goroutine to another, as shown in https://golang.org/cl/134395. I'm no longer certain that it really should be an error to call t.Fatal from another goroutine, provided that that goroutine is appropriately linked back to the test.

@bcmills bcmills mentioned this issue Oct 10, 2018
Closed
@bcmills bcmills closed this as completed Oct 31, 2018
@bcmills bcmills reopened this Oct 31, 2018
@brikis98 brikis98 mentioned this issue Dec 12, 2018
Merged
@ash2k ash2k mentioned this issue Jan 4, 2020
Closed
@cvgw cvgw mentioned this issue Jan 18, 2020
Open
@lrettig lrettig mentioned this issue Sep 25, 2020
Closed
@IronSavior IronSavior mentioned this issue Feb 28, 2023
Open
@bcmills
Copy link
Contributor

bcmills commented Jan 5, 2021

See previously #3800.

@johnstcn johnstcn mentioned this issue May 23, 2022
Merged
@gopherbot
Copy link

Change https://go.dev/cl/416555 mentions this issue: errgroup: propagate panics and goexits from goroutines in the group

@bcmills bcmills mentioned this issue Jul 8, 2022
Open
@bcmills
Copy link
Contributor

bcmills commented Jun 27, 2023

Note that one special case of calling Fatalf in the wrong goroutine is calling a parent's Fatalf from within a subtest; see #22882.

@ash2k ash2k mentioned this issue Oct 31, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests