GODRIVER-3340 Add a test for goroutine leaks. #1874
Conversation
mongodb-drivers-pr-bot
bot
added
dependencies
API Change ReportNo changes found! |
matthewdale
force-pushed
the
experiment-detect-goroutines
branch
5 times, most recently
from
c8d11ec to
e81225e
Compare
matthewdale
force-pushed
the
experiment-detect-goroutines
branch
from
e81225e to
e339231
Compare
blink1073
requested review from
prestonvasquez
and removed request for
blink1073
internal/test/goleak/goleak_test.go
Outdated
| }{ | ||
| { | ||
| desc: "base", | ||
| opts: options.Client().ApplyURI(uri), |
There was a problem hiding this comment.
ApplyURI is used in every test and if it's empty will default to localhost:27017. Suggest applying this value in the test runner:
opts := tc.opts
if u := os.Getenv("MONGODB_URI"); u != "" {
opts.ApplyURI(u)
}
client, err := mongo.Connect(context.Background(), opts)
require.NoError(t, err)There was a problem hiding this comment.
My concern with that approach is that ApplyURI will overwrite the explicitly configured values if it's called after. However, I believe I can get the same effect using something like
base := options.Client()
if u := os.Getenv("MONGODB_URI"); u != "" {
base.ApplyURI(u)
}
client, err := mongo.Connect(context.Background(), base, t.opts)| // These can't be run in parallel because goleak currently can't filter | ||
| // out goroutines from other parallel subtests. | ||
| t.Run(tc.desc, func(t *testing.T) { | ||
| defer goleak.VerifyNone(t) |
There was a problem hiding this comment.
The uber-go/goleak documentation implies that we don't have to make this check both in the subtest and at the TestMain() level: https://github.com/uber-go/goleak#quick-start . If that's true, suggest removing TestMain().
There was a problem hiding this comment.
Good idea. I added it to catch goroutine leaks that I didn't know I was looking for, but it's never caught a goroutine leak that goleak.VerifyNone didn't catch, so I'm fine removing it.
We should definitely prioritize using goleak.VerifyNone in specific test cases because it both indicates which scenario(s) caused a goroutine leak and the output shows up clearly in the Evergreen test logs. goleak.VerifyTestMain does neither.
There was a problem hiding this comment.
Removed TestMain.
internal/test/goleak/goleak_test.go
Outdated
| _, err = coll.InsertOne(context.Background(), bson.M{"x": 123}) | ||
| require.NoError(t, err) | ||
|
|
||
| for i := 0; i < 20; i++ { |
There was a problem hiding this comment.
What is the significance of 20? Should we constantize this value to give it context?
There was a problem hiding this comment.
20 is "enough to cause a noticeable problem", so it doesn't have any special significance. However, note that we've only detected one type of goroutine leak with this test so far, so it's possible that we'll need to change the test scenario, duration, etc to catch future goroutine leaks. I'll leave a comment about the significance and limitations of the overall test loop related to detecting goroutine leaks. Ideally, we should actually be testing for goroutine leaks in our existing test suites, but that has proved somewhat difficult to implement.
There was a problem hiding this comment.
I ended up removing the sleep, increased the iterations to 50, moved the InsertOne into the loop, added a ChangeStream simulated workload, and added some operations that intentionally time out. I added a comment describing the intent of the different operations.
internal/test/goleak/goleak_test.go
Outdated
| var res bson.D | ||
| err = coll.FindOne(context.Background(), bson.D{}).Decode(&res) | ||
| require.NoError(t, err) | ||
| time.Sleep(50 * time.Millisecond) |
There was a problem hiding this comment.
Could we add a comment to describe what we are awaiting?
There was a problem hiding this comment.
For some reason, adding the sleep increases the number of goroutines that leak when using "zstd" compression. I will add a comment describing the current purpose for sleeping there.
|
drivers-pr-bot please backport to master |
|
Sorry, unable to cherry-pick to master, please backport manually. Here are approximate instructions:
|
(cherry picked from commit c3448e6)
GODRIVER-3340
Summary
internal/test/goleakthat tests for goroutine leaks.Background & Motivation
Our usage of the
zstd.Decoderpooled with async.Poolcreated the possibility of a gorutine leak when using the "zstd" compressor. We didn't catch it because we currently don't have any way to detect goroutine leaks. Add a test that uses github.com/uber-go/goleak to check for goroutine leaks.