Skip to content

Commit 3439cef

Browse files
authored
chore: use os.Stdout instead of t.Output() (#8860)
Signed-off-by: zirain <zirain2009@gmail.com>
1 parent 79273b9 commit 3439cef

1 file changed

Lines changed: 18 additions & 24 deletions

File tree

internal/cmd/server_runner_race_test.go

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,40 +20,41 @@ import (
2020
"github.com/envoyproxy/gateway/internal/logging"
2121
)
2222

23-
// TestRunnerGoroutineRace specifically reproduces the data race from CI by
24-
// simulating the exact condition: goroutines logging while test cleanup closes t.Output()
23+
// TestRunnerGoroutineRace tests that background goroutines can safely log after context cancellation.
24+
// This test previously exposed a data race when using t.Output() - the test would complete and
25+
// close t.Output() while background goroutines were still logging. The fix is to use os.Stdout
26+
// instead, which is thread-safe and remains valid after the test completes.
2527
//
26-
// The race happens because runner.Close() is a no-op - it returns immediately
27-
// without waiting for goroutines to finish. When the test ends, cleanup closes
28-
// t.Output() while goroutines are still active.
28+
// Note: In production, certGen is called with cmd.OutOrStdout() which resolves to os.Stdout
29+
// in most cases, so using os.Stdout in tests mirrors real-world behavior.
2930
//
3031
// Run with: go test -race -run TestRunnerGoroutineRace -count=100 ./internal/cmd/
3132
func TestRunnerGoroutineRace(t *testing.T) {
3233
// Skip if not running with race detector
33-
// This test is specifically designed to catch the race
3434
if !testing.Short() {
35-
t.Skip("Run with -race flag to detect the race")
35+
t.Skip("Run with -race flag to verify no race conditions")
3636
}
3737

3838
configHome := t.TempDir()
3939
cfgFileContent := strings.ReplaceAll(fileProviderGatewayConfig, "[CONFIG_HOME_PLACE_HODLER]", configHome)
4040
configPath := path.Join(t.TempDir(), "envoy-gateway.yaml")
4141
require.NoError(t, os.WriteFile(configPath, []byte(cfgFileContent), 0o600))
4242

43-
require.NoError(t, certGen(t.Context(), t.Output(), true, configHome))
43+
require.NoError(t, certGen(t.Context(), os.Stdout, true, configHome))
4444

4545
// Use a context WITHOUT defer cancel to keep goroutines alive longer
4646
ctx, cancel := context.WithCancel(context.Background())
4747

4848
hook := func(c context.Context, cfg *config.Server) error {
49-
cfg.Logger = logging.DefaultLogger(t.Output(), egv1a1.LogLevelInfo)
49+
// Use os.Stdout instead of t.Output() - it's thread-safe and won't cause races
50+
cfg.Logger = logging.DefaultLogger(os.Stdout, egv1a1.LogLevelInfo)
5051
return startRunners(c, cfg, nil)
5152
}
5253

5354
errCh := make(chan error, 1)
5455

5556
go func() {
56-
errCh <- server(ctx, t.Output(), t.Output(), configPath, hook, nil)
57+
errCh <- server(ctx, os.Stdout, os.Stdout, configPath, hook, nil)
5758
}()
5859

5960
// Let runners start and become active
@@ -62,19 +63,13 @@ func TestRunnerGoroutineRace(t *testing.T) {
6263
// Cancel context - triggers shutdown but goroutines may still be logging
6364
cancel()
6465

65-
// Don't wait for server to complete - this creates the race!
66-
// Test will end, cleanup will close t.Output(), while goroutines
67-
// are still running and trying to log
66+
// Wait briefly to see if server completes or if goroutines are still running
6867
select {
6968
case <-errCh:
70-
// Server finished
69+
// Server finished cleanly
7170
case <-time.After(20 * time.Millisecond):
72-
// Timeout - goroutines likely still running
73-
// Test ends here, cleanup starts -> RACE!
71+
// Server still running - this is fine with os.Stdout (no race)
7472
}
75-
76-
// Test ends immediately - race window is NOW
77-
// Without fix: goroutines still running, trying to log to closed t.Output()
7873
}
7974

8075
// TestRunnerGoroutineRaceStress runs multiple quick cycles to maximize
@@ -87,20 +82,21 @@ func TestRunnerGoroutineRaceStress(t *testing.T) {
8782
configPath := path.Join(t.TempDir(), "envoy-gateway.yaml")
8883
require.NoError(t, os.WriteFile(configPath, []byte(cfgFileContent), 0o600))
8984

90-
require.NoError(t, certGen(t.Context(), t.Output(), true, configHome))
85+
require.NoError(t, certGen(t.Context(), os.Stdout, true, configHome))
9186

9287
ctx, cancel := context.WithCancel(context.Background())
9388
defer cancel()
9489

9590
hook := func(c context.Context, cfg *config.Server) error {
96-
cfg.Logger = logging.DefaultLogger(t.Output(), egv1a1.LogLevelInfo)
91+
// Use os.Stdout instead of t.Output() - it's thread-safe and won't cause races
92+
cfg.Logger = logging.DefaultLogger(os.Stdout, egv1a1.LogLevelInfo)
9793
return startRunners(c, cfg, nil)
9894
}
9995

10096
errCh := make(chan error, 1)
10197

10298
go func() {
103-
errCh <- server(ctx, t.Output(), t.Output(), configPath, hook, nil)
99+
errCh <- server(ctx, os.Stdout, os.Stdout, configPath, hook, nil)
104100
}()
105101

106102
// Vary timing to hit different race windows
@@ -111,8 +107,6 @@ func TestRunnerGoroutineRaceStress(t *testing.T) {
111107

112108
err := <-errCh
113109
require.NoError(t, err)
114-
115-
// Race window: goroutines may still be logging
116110
})
117111
}
118112
}

0 commit comments

Comments
 (0)