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

build: test cleanup wrt wg.Done #5746

Closed
chai2010 opened this issue Jun 21, 2013 · 13 comments
Closed

build: test cleanup wrt wg.Done #5746

chai2010 opened this issue Jun 21, 2013 · 13 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.16

Comments

@chai2010
Copy link
Contributor 

just like this test example:

func TestFailed(t *testing.T) {
    var wg sync.WaitGroup
    for i := 0; i < 2; i++ {
        wg.Add(1)
        go func(id int) {
            // defer wg.Done()
            t.Fatalf("TestFailed: id = %v\n", id)
            wg.Done()
        }(i)
    }
    wg.Wait()
}

go test
fatal error: all goroutines are asleep - deadlock!

---

When the t.Fatalf panic, goroutine can't reach wg.Done().
So wg.Wait() deadlock!

There are lots of similar iusses, eg:
http://golang.org/src/pkg/net/rpc/server_test.go?s=12968:13320#L548

If the issue verified, i can submit patch.
@davecheney
Copy link
Contributor

Comment 1:

That benchmark code is broken, Fatalf should not be called from a goroutine, only the
testing goroutine.
If you want to propose a fix for this, please follow the contribution guidelines here,
http://golang.org/doc/contribute.html.

Labels changed: added priority-later, removed priority-triage.

Status changed to HelpWanted.

@chai2010
Copy link
Contributor Author

Comment 2:

if the Fatalf can't be called from a goroutine,
how to report the other goroutine Fatal infomation?
I look over the `testing.T` and `testing.T` source code,
the `{t|b}.Fatalf` is thread safe.
can we use `defer wg.Done()` to fix these issue(net/rpc/server_test.go [1])?
for p := 0; p < procs; p++ {
    go func() {
        defer wg.Done()
        reply := new(Reply)
        for atomic.AddInt32(&N, -1) >= 0 {
            err := client.Call("Arith.Add", args, reply)
            if err != nil {
                b.Fatalf("rpc error: Add: expected no error but got string %q", err.Error())
            }
            if reply.C != args.A+args.B {
                b.Fatalf("rpc error: Add: expected %d got %d", reply.C, args.A+args.B)
            }
        }
    }()
}
---
[1]
http://golang.org/src/pkg/net/rpc/server_test.go?s=12968:13320#L548

@davecheney
Copy link
Contributor

Comment 3:

You must use t.Errorf, but your suggestion to use defer wg.Done() is better.

@chai2010
Copy link
Contributor Author

Comment 4:

If we use t.Errorf, the goroutine will continues execution.
If test failed, i think we need stop the goroutine execution.
t.Errorf is equivalent to t.Logf followed by t.Fail.
t.Fatalf is equivalent to t.Logf followed by t.FailNow.
So, i think t.Fatalf is better than t.Errorf.

@cznic
Copy link
Contributor

cznic commented Jun 21, 2013

Comment 5:

@4: "If we use t.Errorf, the goroutine will continues execution."
I think one can safely do:
    t.Errorf(whatever, foo)
    return

@chai2010
Copy link
Contributor Author

Comment 6:

the patch has been sent as https://golang.org/cl/10454043/

@davecheney
Copy link
Contributor

Comment 7:

Status changed to Started.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 9:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 10:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 11:

Labels changed: added repo-main.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@bradfitz bradfitz removed the Started label Jan 6, 2017
@gopherbot
Copy link

Change https://golang.org/cl/212920 mentions this issue: analysis/passes: report testing.Fatal* FailNow Skip* misuse in goroutines

gopherbot pushed a commit to golang/tools that referenced this issue Jan 7, 2020
@odeke-em
analysis/passes: report testing.Fatal* FailNow Skip* misuse in gorout…
53017a3 
…ines

Adds an analyzer to report an error if any tests or benchmarks
have any *Fatal, FailNow, Skip* misuses in goroutines which are
forbidden by the package testing, since those functions terminate
the entire benchmark/test yet ideally one goroutine exiting shouldn't
affect the entire benchmark/test.

This first pass only works for plain goroutines and doesn't yet work
with b.RunParallel. That'll be added in a subsequent CL after this
one is reviewed and merged.

Updates golang/go#5746

Change-Id: Ia47e5c9fd96ceced1ae9834b94f529f6ae2edaaa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212920
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
@odeke-em
Copy link
Member

odeke-em commented Jan 7, 2020

Thank you for this bug report @chai2010 and everyone for responding! I examined this issue last week and sent CL 212920 for a go vet analysis pass that will be added in Go1.15 and has been merge into x/tools currently. It also found 3 misuses in the standard library as per this CL 213097

I am going to close this issue since it was more of a question that was answered in the replies, it is also now answered in the documents for testing.T and testing.B but also as mentioned above, go vet will start flagging those cases in >=Go1.15.

Thanks y'all and happy New Year!

@odeke-em odeke-em closed this as completed Jan 7, 2020
@gopherbot
Copy link

Change https://golang.org/cl/235677 mentions this issue: cmd/vet: add pass to catch invalid uses of testing.T in goroutines

gopherbot pushed a commit that referenced this issue Nov 1, 2020
@odeke-em
cmd/vet: bring in pass to catch invalid uses of testing.T in goroutines
5a267c8 
Add "go/analysis/passes/testinggoroutine" from x/tools and vendor its source in.
This pass will catch misuses of:
* testing.T.Fail*
* testing.T.Fatal*
* testing.T.Skip*
inside goroutines explicitly started by the go keyword.

The pass was implemented in CL 212920.

While here, found 2 misuses in:
* database/sql/sql_test.go
* runtime/syscall_windows_test.go
and fixed them in CL 235527.

Fixes #5746

Change-Id: I1740ad3f1d677bb5d78dc5d8d66bac6ec287a2b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/235677
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 2, 2020
@dmitshur dmitshur modified the milestones: Unplanned, Go1.16 Dec 2, 2020
@golang golang locked and limited conversation to collaborators Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
8 participants
@bradfitz @davecheney @rsc @cznic @dmitshur @chai2010 @odeke-em @gopherbot