Skip to content

Commit ea2aa0e

Browse files
committed
Fix test races by ensuring goroutines do not outlive their It blocks.
1 parent ba51d28 commit ea2aa0e

File tree

2 files changed

+33
-14
lines changed

2 files changed

+33
-14
lines changed

pkg/internal/controller/controller.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ func (c *Controller[request]) Warmup(ctx context.Context) error {
234234
c.mu.Lock()
235235
defer c.mu.Unlock()
236236

237-
// Set the ctx so later calls to watch use this internal context of nil
237+
// Set the ctx so later calls to watch use this internal context
238238
c.ctx = ctx
239239

240240
return c.startEventSourcesLocked(ctx)

pkg/internal/controller/controller_test.go

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -549,15 +549,22 @@ var _ = Describe("controller", func() {
549549
ctrl.startWatches = []source.TypedSource[reconcile.Request]{src}
550550

551551
By("Calling startEventSourcesLocked asynchronously")
552+
wg := sync.WaitGroup{}
552553
go func() {
553554
defer GinkgoRecover()
555+
defer wg.Done()
556+
557+
wg.Add(1)
554558
Expect(ctrl.startEventSourcesLocked(ctx)).To(Succeed())
555559
}()
556560

557561
By("Calling startEventSourcesLocked again")
558562
var didSubsequentCallComplete atomic.Bool
559563
go func() {
560564
defer GinkgoRecover()
565+
defer wg.Done()
566+
567+
wg.Add(1)
561568
Expect(ctrl.startEventSourcesLocked(ctx)).To(Succeed())
562569
didSubsequentCallComplete.Store(true)
563570
}()
@@ -570,6 +577,7 @@ var _ = Describe("controller", func() {
570577

571578
// Assert that second call to startEventSourcesLocked is now complete
572579
Eventually(didSubsequentCallComplete.Load).Should(BeTrue(), "startEventSourcesLocked should complete after source is started and synced")
580+
wg.Wait()
573581
})
574582

575583
It("should reset c.startWatches to nil after returning", func() {
@@ -1155,30 +1163,26 @@ var _ = Describe("controller", func() {
11551163
}),
11561164
}
11571165

1158-
// Wait for the goroutines to finish before returning to avoid racing with the
1159-
// assignment in BeforeEach block.
1160-
var wg sync.WaitGroup
1166+
// channel to prevent the goroutine from outliving the It test
1167+
waitChan := make(chan struct{})
11611168

11621169
// Invoked in a goroutine because Warmup will block
1163-
wg.Add(1)
11641170
go func() {
11651171
defer GinkgoRecover()
1166-
defer wg.Done()
1172+
defer close(waitChan)
11671173
Expect(ctrl.Warmup(ctx)).To(Succeed())
11681174
}()
11691175

11701176
cancel()
1171-
wg.Wait()
1177+
<-waitChan
11721178
})
11731179

11741180
It("should be called before leader election runnables if warmup is enabled", func() {
11751181
// This unit test exists to ensure that a warmup enabled controller will actually be
11761182
// called in the warmup phase before the leader election runnables are started. It
11771183
// catches regressions in the controller that would not implement warmupRunnable from
11781184
// pkg/manager.
1179-
11801185
ctx, cancel := context.WithCancel(context.Background())
1181-
defer cancel()
11821186

11831187
By("Creating a channel to track execution order")
11841188
runnableExecutionOrderChan := make(chan string, 2)
@@ -1229,14 +1233,19 @@ var _ = Describe("controller", func() {
12291233
Expect(m.Add(nonWarmupCtrl)).To(Succeed())
12301234

12311235
By("Starting the manager")
1236+
waitChan := make(chan struct{})
12321237
go func() {
12331238
defer GinkgoRecover()
1239+
defer close(waitChan)
12341240
Expect(m.Start(ctx)).To(Succeed())
12351241
}()
12361242

12371243
<-m.Elected()
12381244
Expect(<-runnableExecutionOrderChan).To(Equal(warmupRunnableName))
12391245
Expect(<-runnableExecutionOrderChan).To(Equal(nonWarmupRunnableName))
1246+
1247+
cancel()
1248+
<-waitChan
12401249
})
12411250

12421251
It("should not cause a data race when called concurrently", func() {
@@ -1299,7 +1308,6 @@ var _ = Describe("controller", func() {
12991308

13001309
It("should start sources added after Warmup is called", func() {
13011310
ctx, cancel := context.WithCancel(context.Background())
1302-
defer cancel()
13031311

13041312
ctrl.CacheSyncTimeout = time.Second
13051313

@@ -1312,12 +1320,17 @@ var _ = Describe("controller", func() {
13121320
return nil
13131321
}))).To(Succeed())
13141322

1323+
waitChan := make(chan struct{})
13151324
go func() {
13161325
defer GinkgoRecover()
13171326
Expect(ctrl.Start(ctx)).To(Succeed())
1327+
close(waitChan)
13181328
}()
13191329

13201330
Eventually(didWatchStart.Load).Should(BeTrue(), "watch should be started if it is added after Warmup")
1331+
1332+
cancel()
1333+
<-waitChan
13211334
})
13221335

13231336
DescribeTable("should not leak goroutines when manager is stopped with warmup runnable",
@@ -1349,19 +1362,22 @@ var _ = Describe("controller", func() {
13491362
// ignore needs to go after the testenv.Start() call to ignore the apiserver
13501363
// process
13511364
currentGRs := goleak.IgnoreCurrent()
1365+
waitChan := make(chan struct{})
13521366
go func() {
13531367
defer GinkgoRecover()
13541368
Expect(m.Start(ctx)).To(Succeed())
1369+
close(waitChan)
13551370
}()
13561371

13571372
<-m.Elected()
13581373
By("stopping the manager via context")
13591374
cancel()
13601375

13611376
Eventually(func() error { return goleak.Find(currentGRs) }).Should(Succeed())
1377+
<-waitChan
13621378
},
1363-
Entry("manager with leader election enabled", true),
1364-
Entry("manager without leader election enabled", false),
1379+
Entry("and with leader election enabled", true),
1380+
Entry("and without leader election enabled", false),
13651381
)
13661382
})
13671383

@@ -1373,7 +1389,6 @@ var _ = Describe("controller", func() {
13731389
It("should not start sources when Warmup is called if warmup is disabled but start it when Start is called.", func() {
13741390
// Setup controller with sources that complete successfully
13751391
ctx, cancel := context.WithCancel(context.Background())
1376-
defer cancel()
13771392

13781393
ctrl.CacheSyncTimeout = time.Second
13791394
var isSourceStarted atomic.Bool
@@ -1391,12 +1406,16 @@ var _ = Describe("controller", func() {
13911406
Expect(isSourceStarted.Load()).To(BeFalse())
13921407

13931408
By("Calling Start when EnableWarmup is false")
1394-
// Now call Start
1409+
waitChan := make(chan struct{})
1410+
13951411
go func() {
13961412
defer GinkgoRecover()
13971413
Expect(ctrl.Start(ctx)).To(Succeed())
1414+
close(waitChan)
13981415
}()
13991416
Eventually(isSourceStarted.Load).Should(BeTrue())
1417+
cancel()
1418+
<-waitChan
14001419
})
14011420
})
14021421
})

0 commit comments

Comments
 (0)