From 313aed58be3961d53e716310276ee8ec9ceb5473 Mon Sep 17 00:00:00 2001 From: Brandur Date: Sun, 17 Dec 2023 12:41:41 -0800 Subject: [PATCH] Fix data race in rescuer Here, the data race described here in the rescuer. It's a little subtle, but the basic problem is that `defer` statements are LIFO, so what can happen is that the rescuer can be closed while there's still a log statement deferred on the stack. Because the log statements feed to `t.Logf` through a shunt, this can occasionally cause a log line to be sent to test output after the test has ended, causing the race detector to detect a race. The solution is to `defer` the close operation before the log operation. All the other maintenance services already had this race condition patched quite some time ago, but the fix didn't make it to the rescuer, probably due to an outdated branch or the like. [1] https://github.com/riverqueue/river/pull/116#issuecomment-1858986062 --- internal/maintenance/rescuer.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/maintenance/rescuer.go b/internal/maintenance/rescuer.go index 8da51b98..aaef7d24 100644 --- a/internal/maintenance/rescuer.go +++ b/internal/maintenance/rescuer.go @@ -112,11 +112,13 @@ func (s *Rescuer) Start(ctx context.Context) error { s.CancellableSleepRandomBetween(ctx, JitterMin, JitterMax) go func() { + // This defer should come first so that it's last out, thereby avoiding + // races. + defer close(stopped) + s.Logger.InfoContext(ctx, s.Name+logPrefixRunLoopStarted) defer s.Logger.InfoContext(ctx, s.Name+logPrefixRunLoopStopped) - defer close(stopped) - ticker := timeutil.NewTickerWithInitialTick(ctx, s.Config.Interval) for { select {