From 8b344bf3dc20d056fb5cd21adc73edb52a05715c Mon Sep 17 00:00:00 2001 From: Julien Duchesne Date: Fri, 4 Oct 2024 14:13:57 -0400 Subject: [PATCH 1/3] Fix flaky `Test flake TestReusableGoroutinesPool` test Fixes https://github.com/grafana/dskit/issues/593 Count goroutines from the current test rather than for the whole process --- concurrency/worker_test.go | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/concurrency/worker_test.go b/concurrency/worker_test.go index c26529a55..4be57e554 100644 --- a/concurrency/worker_test.go +++ b/concurrency/worker_test.go @@ -1,7 +1,9 @@ package concurrency import ( + "regexp" "runtime" + "strings" "testing" "time" @@ -9,40 +11,53 @@ import ( ) func TestReusableGoroutinesPool(t *testing.T) { - baseGoroutines := runtime.NumGoroutine() - const workers = 2 + buf := make([]byte, 1<<20) + buf = buf[:runtime.Stack(buf, false)] + testGoroutine := regexp.MustCompile(`goroutine (\d+) \[running\]:`).FindSubmatch(buf)[1] + require.NotEmpty(t, testGoroutine, "test goroutine not found") + + countGoroutines := func() int { + buf := make([]byte, 1<<20) + buf = buf[:runtime.Stack(buf, true)] + // Count the number of goroutines created by this test + // This ensures that the test isn't affected by leaked goroutines from other tests. + return strings.Count(string(buf), " in goroutine "+string(testGoroutine)) + } + baseGoroutines := countGoroutines() + const workers = 2 w := NewReusableGoroutinesPool(workers) - require.Equal(t, baseGoroutines+workers, runtime.NumGoroutine()) + + require.Equal(t, baseGoroutines+workers, countGoroutines()) // Wait a little bit so both goroutines would be waiting on the jobs chan. time.Sleep(10 * time.Millisecond) ch := make(chan struct{}) w.Go(func() { <-ch }) - require.Equal(t, baseGoroutines+workers, runtime.NumGoroutine()) + require.Equal(t, baseGoroutines+workers, countGoroutines()) w.Go(func() { <-ch }) - require.Equal(t, baseGoroutines+workers, runtime.NumGoroutine()) + require.Equal(t, baseGoroutines+workers, countGoroutines()) w.Go(func() { <-ch }) - require.Equal(t, baseGoroutines+workers+1, runtime.NumGoroutine()) + require.Equal(t, baseGoroutines+workers+1, countGoroutines()) // end workloads, we should have only the workers again. close(ch) for i := 0; i < 1000; i++ { - if runtime.NumGoroutine() == baseGoroutines+workers { + if countGoroutines() == baseGoroutines+workers { break } time.Sleep(time.Millisecond) } - require.Equal(t, baseGoroutines+workers, runtime.NumGoroutine()) + require.Equal(t, baseGoroutines+workers, countGoroutines()) // close the workers, eventually they should be gone. w.Close() for i := 0; i < 1000; i++ { - if runtime.NumGoroutine() == baseGoroutines { + if countGoroutines() == baseGoroutines { return } time.Sleep(time.Millisecond) } - t.Fatalf("expected %d goroutines after closing, got %d", baseGoroutines, runtime.NumGoroutine()) + t.Fatalf("expected %d goroutines after closing, got %d", baseGoroutines, countGoroutines()) } From 12bb935068af368df317575b6e542781c47681a8 Mon Sep 17 00:00:00 2001 From: Julien Duchesne Date: Fri, 4 Oct 2024 15:05:29 -0400 Subject: [PATCH 2/3] Avoid concurrency groups on flaky test finder --- .github/workflows/find-flaky-tests.yml | 2 ++ .github/workflows/test-build.yml | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/find-flaky-tests.yml b/.github/workflows/find-flaky-tests.yml index feade8344..c3a41d668 100644 --- a/.github/workflows/find-flaky-tests.yml +++ b/.github/workflows/find-flaky-tests.yml @@ -12,3 +12,5 @@ jobs: matrix: runs: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20] # Run the build workflow 20 times uses: ./.github/workflows/test-build.yml + with: + concurrency-group: '-find-flaky-tests-${{ matrix.runs }}' diff --git a/.github/workflows/test-build.yml b/.github/workflows/test-build.yml index d80349196..e07dd89ce 100644 --- a/.github/workflows/test-build.yml +++ b/.github/workflows/test-build.yml @@ -5,12 +5,16 @@ on: - main pull_request: workflow_call: + inputs: + concurrency-group: + required: true + type: string concurrency: # Cancel any running workflow for the same branch when new commits are pushed. # We group both by ref_name (available when CI is triggered by a push to a branch/tag) # and head_ref (available when CI is triggered by a PR). - group: "${{ github.ref_name }}-${{ github.head_ref }}" + group: "${{ github.ref_name }}-${{ github.head_ref }}${{ inputs.concurrency-group }}" cancel-in-progress: true jobs: From a982594a4960416b9aebbf959f6ce9720d6e2bbd Mon Sep 17 00:00:00 2001 From: Julien Duchesne Date: Fri, 4 Oct 2024 15:14:06 -0400 Subject: [PATCH 3/3] Get rid of `baseGoroutines`. It's now zero --- concurrency/worker_test.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/concurrency/worker_test.go b/concurrency/worker_test.go index 4be57e554..338062055 100644 --- a/concurrency/worker_test.go +++ b/concurrency/worker_test.go @@ -24,40 +24,38 @@ func TestReusableGoroutinesPool(t *testing.T) { return strings.Count(string(buf), " in goroutine "+string(testGoroutine)) } - baseGoroutines := countGoroutines() - const workers = 2 - w := NewReusableGoroutinesPool(workers) - - require.Equal(t, baseGoroutines+workers, countGoroutines()) + const workerCount = 2 + w := NewReusableGoroutinesPool(workerCount) + require.Equal(t, workerCount, countGoroutines()) // Wait a little bit so both goroutines would be waiting on the jobs chan. time.Sleep(10 * time.Millisecond) ch := make(chan struct{}) w.Go(func() { <-ch }) - require.Equal(t, baseGoroutines+workers, countGoroutines()) + require.Equal(t, workerCount, countGoroutines()) w.Go(func() { <-ch }) - require.Equal(t, baseGoroutines+workers, countGoroutines()) + require.Equal(t, workerCount, countGoroutines()) w.Go(func() { <-ch }) - require.Equal(t, baseGoroutines+workers+1, countGoroutines()) + require.Equal(t, workerCount+1, countGoroutines()) // end workloads, we should have only the workers again. close(ch) for i := 0; i < 1000; i++ { - if countGoroutines() == baseGoroutines+workers { + if countGoroutines() == workerCount { break } time.Sleep(time.Millisecond) } - require.Equal(t, baseGoroutines+workers, countGoroutines()) + require.Equal(t, workerCount, countGoroutines()) // close the workers, eventually they should be gone. w.Close() for i := 0; i < 1000; i++ { - if countGoroutines() == baseGoroutines { + if countGoroutines() == 0 { return } time.Sleep(time.Millisecond) } - t.Fatalf("expected %d goroutines after closing, got %d", baseGoroutines, countGoroutines()) + t.Fatalf("expected %d goroutines after closing, got %d", 0, countGoroutines()) }