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: refactoring leads to deadlocks #64402

Closed
1 task done
lfolger opened this issue Nov 27, 2023 · 4 comments
Closed
1 task done

testing: refactoring leads to deadlocks #64402

lfolger opened this issue Nov 27, 2023 · 4 comments
Assignees
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@lfolger
Copy link
Contributor

lfolger commented Nov 27, 2023

Go version

tip

Reproducibility

  • Does this issue reproduce with the latest release?

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

linux/amd64

What did you do?

The refactoring of the testing package in https://go.dev/cl/506755 leads to deadlocks.

One example that triggers this is:

func TestTesting(t *testing.T) { // <=== this is the outer t
    t.Run("outer", func(*testing.T) {
        t.Run("inner", func(t *testing.zT) { // <=== This is using the outer t
            t.Log("Hello World!")
        })
    })
}

In our Google internal testing we discovered other cases but I don't yet know a minimal reproducer that I can share here. I'll follow up separately.

What did you expect to see?

I expected the test to complete successfully.

What did you see instead?

The tests deadlocks.

@bcmills bcmills added this to the Go1.22 milestone Nov 27, 2023
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 27, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/544895 mentions this issue: Revert "testing: simplify concurrency and cleanup logic"

gopherbot pushed a commit that referenced this issue Nov 27, 2023
reverts CL 506755

Reason for revert: This leads to deadlocks of tests in Google internal testing

For #64402

Change-Id: I78329fc9dcc2377e7e880b264ac1d18d577ef99a
Reviewed-on: https://go-review.googlesource.com/c/go/+/544895
Auto-Submit: Bryan Mills <bcmills@google.com>
Auto-Submit: Lasse Folger <lassefolger@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/545735 mentions this issue: testing: lock-in a deadlocking regression test that failed after a refactor

@odeke-em
Copy link
Member

Thank you for reporting this regression @lfolger, the CL https://go-review.googlesource.com/c/go/+/506755 got reverted by you in https://go-review.googlesource.com/c/go/+/544895. I've mailed a CL to lock-in your test per https://go-review.googlesource.com/c/go/+/545735 and that should then close this issue.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/546019 mentions this issue: testing: add regression tests for reentrant calls to T.Run

@golang golang locked and limited conversation to collaborators Nov 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants