Skip to content

Commit

Permalink
fix: Fix flaky blocklist test (#52)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?

This is re:
#45 (comment)

The original code has a rare race condition:

1) On line 180, the `currentIndex` is loaded
2) Context switch to where `currentIndex` is incremented
3) `IncrementKey` is called on the updated index
4) `AggregateCounts` is called on an old `currentIndex`. This is
undefined behavior, as `currentIndex` is assumed to be monotonically
increasing.

To validate that this race condition was fixed, I wrote a script that
ran the test 100 times.

With the previous code, I observed 7 failures out of 100 runs.

With the new code, I observed 0 failures out of 100 runs.

## Short description of the changes

Small change to unit tests.

Co-authored-by: Yi Zhao <yizhao@stripe.com>
  • Loading branch information
yizzlez and yizhao-stripe authored Mar 10, 2023
1 parent 1bbf5b6 commit 1e7ae5a
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions blocklist_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,12 @@ func compareConcurrency(t *testing.T, reference BlockList, actual BlockList) {
case <-done:
return
case <-updateTicker.C:
currentIndex := atomic.LoadInt64(&globalIndex)

lock.Lock()
currentIndex := atomic.LoadInt64(&globalIndex)
referenceAggregate := reference.AggregateCounts(currentIndex, 10)
actualAggregate := actual.AggregateCounts(currentIndex, 10)

assert.Equal(t, referenceAggregate, actualAggregate)
lock.Unlock()
}
Expand All @@ -194,13 +195,14 @@ func compareConcurrency(t *testing.T, reference BlockList, actual BlockList) {
go func() {
defer wg.Done()
for j := 0; j < iterations; j++ {
currentIndex := atomic.LoadInt64(&globalIndex)

// These need to be performed atomically.
lock.Lock()
currentIndex := atomic.LoadInt64(&globalIndex)
referenceErr := reference.IncrementKey(testKey, currentIndex)
actualErr := actual.IncrementKey(testKey, currentIndex)
assert.Equal(t, referenceErr, actualErr)

sleepTime := time.Duration(random.Intn(100)) * time.Millisecond
lock.Unlock()

Expand Down

0 comments on commit 1e7ae5a

Please sign in to comment.