From 2aad4d6b5d4d2850a2c89cc78cb0c50086e22421 Mon Sep 17 00:00:00 2001 From: Pradyumna Agrawal Date: Mon, 7 Dec 2020 10:10:04 -0800 Subject: [PATCH 1/4] Fix race between `OnDemandHousekeeping` and `housekeepingTick` This fixes the data race between the functions which could be running in different goroutines. Signed-off-by: Pradyumna Agrawal --- manager/container.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/manager/container.go b/manager/container.go index c599300d84..721019df18 100644 --- a/manager/container.go +++ b/manager/container.go @@ -138,7 +138,10 @@ func (cd *containerData) allowErrorLogging() bool { // periodic housekeeping to reset. This should be used sparingly, as calling OnDemandHousekeeping frequently // can have serious performance costs. func (cd *containerData) OnDemandHousekeeping(maxAge time.Duration) { - if cd.clock.Since(cd.statsLastUpdatedTime) > maxAge { + cd.lock.Lock() + timeSinceStatsLastUpdate := cd.clock.Since(statsLastUpdatedTime) + cd.lock.Unock() + if timeSinceStatsLastUpdate > maxAge { housekeepingFinishedChan := make(chan struct{}) cd.onDemandChan <- housekeepingFinishedChan select { @@ -555,6 +558,8 @@ func (cd *containerData) housekeepingTick(timer <-chan time.Time, longHousekeepi klog.V(3).Infof("[%s] Housekeeping took %s", cd.info.Name, duration) } cd.notifyOnDemand() + cd.lock.Lock() + defer cd.lock.Unock() cd.statsLastUpdatedTime = cd.clock.Now() return true } From 8ab935652d575d62bf776db6eb73ef5a2c028fdf Mon Sep 17 00:00:00 2001 From: Pradyumna Agrawal Date: Mon, 7 Dec 2020 10:15:39 -0800 Subject: [PATCH 2/4] Fix typo in previous commit Signed-off-by: Pradyumna Agrawal --- manager/container.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/manager/container.go b/manager/container.go index 721019df18..fef53f4d28 100644 --- a/manager/container.go +++ b/manager/container.go @@ -140,7 +140,7 @@ func (cd *containerData) allowErrorLogging() bool { func (cd *containerData) OnDemandHousekeeping(maxAge time.Duration) { cd.lock.Lock() timeSinceStatsLastUpdate := cd.clock.Since(statsLastUpdatedTime) - cd.lock.Unock() + cd.lock.Unlock() if timeSinceStatsLastUpdate > maxAge { housekeepingFinishedChan := make(chan struct{}) cd.onDemandChan <- housekeepingFinishedChan @@ -559,7 +559,7 @@ func (cd *containerData) housekeepingTick(timer <-chan time.Time, longHousekeepi } cd.notifyOnDemand() cd.lock.Lock() - defer cd.lock.Unock() + defer cd.lock.Unlock() cd.statsLastUpdatedTime = cd.clock.Now() return true } From fe157a0f9d015ed5f1dd91638fd8037503e788f3 Mon Sep 17 00:00:00 2001 From: Pradyumna Agrawal Date: Mon, 7 Dec 2020 10:27:40 -0800 Subject: [PATCH 3/4] cd.clock.Since(statsLastUpdatedTime) -> cd.clock.Since(cd.statsLastUpdatedTime) Signed-off-by: Pradyumna Agrawal --- manager/container.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/manager/container.go b/manager/container.go index fef53f4d28..688e3564fd 100644 --- a/manager/container.go +++ b/manager/container.go @@ -139,7 +139,7 @@ func (cd *containerData) allowErrorLogging() bool { // can have serious performance costs. func (cd *containerData) OnDemandHousekeeping(maxAge time.Duration) { cd.lock.Lock() - timeSinceStatsLastUpdate := cd.clock.Since(statsLastUpdatedTime) + timeSinceStatsLastUpdate := cd.clock.Since(cd.statsLastUpdatedTime) cd.lock.Unlock() if timeSinceStatsLastUpdate > maxAge { housekeepingFinishedChan := make(chan struct{}) From 409ce2c271b4ec50cf624e005c001b4ea17b809f Mon Sep 17 00:00:00 2001 From: Pradyumna Agrawal Date: Tue, 29 Dec 2020 14:52:06 -0800 Subject: [PATCH 4/4] add a test case Signed-off-by: Pradyumna Agrawal --- manager/container_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/manager/container_test.go b/manager/container_test.go index cdb1228ca2..63872ed6ea 100644 --- a/manager/container_test.go +++ b/manager/container_test.go @@ -319,3 +319,34 @@ func TestOnDemandHousekeepingReturnsAfterStopped(t *testing.T) { mockHandler.AssertExpectations(t) } + +func TestOnDemandHousekeepingRace(t *testing.T) { + statsList := itest.GenerateRandomStats(1, 4, 1*time.Second) + stats := statsList[0] + + cd, mockHandler, _, _ := newTestContainerData(t) + mockHandler.On("GetStats").Return(stats, nil) + + wg := sync.WaitGroup{} + wg.Add(1002) + + go func() { + time.Sleep(10 * time.Millisecond) + err := cd.Start() + assert.NoError(t, err) + wg.Done() + }() + + go func() { + t.Log("starting on demand goroutine") + for i := 0; i < 1000; i++ { + go func() { + time.Sleep(1 * time.Microsecond) + cd.OnDemandHousekeeping(0 * time.Millisecond) + wg.Done() + }() + } + wg.Done() + }() + wg.Wait() +}