Skip to content

Commit

Permalink
Fix race condition in DoesNotHave (#1287)
Browse files Browse the repository at this point in the history
There is a race condition in the `t.bf` variable if `table.LoadBloomsOnOpen` is set to `false`.
This issue was also seen in hypermodeinc/dgraph#4689 (comment)
  • Loading branch information
Ibrahim Jarif authored Apr 8, 2020
1 parent fa94030 commit 6e032c0
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 0 deletions.
5 changes: 5 additions & 0 deletions table/table.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ type Table struct {

fd *os.File // Own fd.
tableSize int // Initialized in OpenTable, using fd.Stat().
bfLock sync.Mutex

blockIndex []*pb.BlockOffset
ref int32 // For file garbage collection. Atomic.
Expand Down Expand Up @@ -381,7 +382,9 @@ func (t *Table) readIndex() error {
t.blockIndex = index.Offsets

if t.opt.LoadBloomsOnOpen {
t.bfLock.Lock()
t.bf, _ = t.readBloomFilter()
t.bfLock.Unlock()
}

return nil
Expand Down Expand Up @@ -511,11 +514,13 @@ func (t *Table) DoesNotHave(hash uint64) bool {

// Return fast if the cache is absent.
if t.opt.BfCache == nil {
t.bfLock.Lock()
// Load bloomfilter into memory if the cache is absent.
if t.bf == nil {
y.AssertTrue(!t.opt.LoadBloomsOnOpen)
t.bf, _ = t.readBloomFilter()
}
t.bfLock.Unlock()
return !t.bf.Has(hash)
}

Expand Down
20 changes: 20 additions & 0 deletions table/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"os"
"sort"
"strings"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -932,3 +933,22 @@ func TestOpenKVSize(t *testing.T) {
var entrySize uint64 = 15 /* DiffKey len */ + 4 /* Header Size */ + 4 /* Encoded vp */
require.Equal(t, entrySize, table.EstimatedSize())
}

// Run this test with command "go test -race -run TestDoesNotHaveRace"
func TestDoesNotHaveRace(t *testing.T) {
opts := getTestTableOptions()
f := buildTestTable(t, "key", 10000, opts)
table, err := OpenTable(f, opts)
require.NoError(t, err)
defer table.DecrRef()

var wg sync.WaitGroup
wg.Add(5)
for i := 0; i < 5; i++ {
go func() {
require.True(t, table.DoesNotHave(uint64(1237882)))
wg.Done()
}()
}
wg.Wait()
}

0 comments on commit 6e032c0

Please sign in to comment.