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: document cap the number of concurrent goroutines with -race #23611

Closed
ericlagergren opened this issue Jan 30, 2018 · 11 comments
Closed
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ericlagergren
Copy link
Contributor

ericlagergren commented Jan 30, 2018

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

1.9.2
GOOS=linux
GOARCH=amd64

What did you do?

package main

import (
	"math/rand"
	"strconv"
	"testing"
	"time"
)

func TestGoRoutines(t *testing.T) {
	for i := 0; i < 1<<15; i++ {
		t.Run(strconv.Itoa(i), func(t *testing.T) {
			t.Parallel()
			time.Sleep(time.Duration(rand.Intn(500000000)))
		})
	}
}

saved as x_test.go and run with go test -race

What did you expect to see?

The tests to pass. Instead,

race: limit on 8192 simultaneously alive goroutines is exceeded, dying

As I understand it, this is likely a technical limitation on Go's race detector. However, it would be nice for the testing package to be aware of this and, if raceenabled, cap the number of parallel tests at something below the 8192 limit (say, 7000?). Perhaps a better way would be a warning, but I know Go doesn't really like warnings.

@ianlancetaylor
Copy link
Contributor

I'm curious as to whether and how this arose in reality. Do you have tests that intentionally use t.Parallel to run 8192 concurrent goroutines?

@ericlagergren
Copy link
Contributor Author

ericlagergren commented Jan 30, 2018

Yeah, so originally I had N tests each with M subtests, not too dissimilar from this example in the testing docs:

func TestGroupedParallel(t *testing.T) {
    for _, tc := range tests {
        tc := tc // capture range variable
        t.Run(tc.Name, func(t *testing.T) {
            t.Parallel()
            ...
        })
    }
}

Running the tests that way allowed them to complete in half the time (something like 1 instead of 2 minutes).

After drastically increasing the number of (auto-generated) M, I started running into mysterious TravisCI errors which I then discovered was the 8192 goroutine limit.

@ianlancetaylor
Copy link
Contributor

I don't think is possible. There is already a limit on the number of tests that are run in parallel, controlled by the -test.parallel option, whose default is runtime.GOMAXPROCS(0). It doesn't help, though, because by the time we know that the test is going to run in parallel we have already started it. We can't not start a test, because then we would never finish running all the non-parallel tests.

I'm going to close this as impossible but I'd be willing to hear a proposal for how to implement it.

@ericlagergren
Copy link
Contributor Author

ericlagergren commented Mar 28, 2018

@ianlancetaylor fair enough. Could this be a doc fix, though? It was confusing the first time I stumbled upon it. I mean, the testing docs themselves give an example (which I quoted above) that could easily be used to hit the 8,192 limit. I'd be down to submit a doc fix.

@ianlancetaylor
Copy link
Contributor

I'm OK with a doc fix but I'm not sure where it should go. Maybe in https://blog.golang.org/race-detector ?

@ericlagergren
Copy link
Contributor Author

ericlagergren commented Mar 28, 2018

Throwing a couple ideas out there:

  • A comment under theTestGroupedParallel example that warns against using too many parallel tests if race is enabled. Perhaps something like, "Note: because it can be easy to run many parallel tests, care should be taken when the -race flag is toggled, as Go's race detector will kill the program if more than 8,192 goroutines are run simultaneously."

  • A // BUG(...) comment which warns against it in testing. (But, it's not really a bug per se...)

  • An addendum to the blog post is a good idea, but I've a feeling most people won't see it. (Could be mitigated if the text race: limit on 8192 simultaneously alive goroutines is exceeded, dying is added verbatim, since people will likely copy and paste that into Google.)

@ianlancetaylor
Copy link
Contributor

I'm fine with a comment in TestGroupedParallel, want to send a pull request?

@ianlancetaylor ianlancetaylor added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Mar 28, 2018
@ianlancetaylor ianlancetaylor changed the title testing: consider capping the number of concurrent goroutines testing: document cap the number of concurrent goroutines with -race Mar 28, 2018
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Mar 28, 2018
@ericlagergren
Copy link
Contributor Author

@ianlancetaylor yessir. I'll get to it tonight. Do you want it inside TestGroupedParallel or just above/below it?

@ianlancetaylor
Copy link
Contributor

I would guess below it, actually.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/103160 mentions this issue: testing: document that -race can cause affect how many parallel tests

@ericlagergren
Copy link
Contributor Author

ericlagergren commented Mar 29, 2018

@ianlancetaylor great that's where I added it. And darn, I should've tried making it with the new GitHub deal instead of git-codereview :)

@golang golang locked and limited conversation to collaborators Mar 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants