From 82b7845ac4223a50acdc007f167bcf58a0085793 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Wed, 1 Apr 2020 23:24:28 +0530 Subject: [PATCH 1/4] Fix race condition in DoesNotHave --- table/table.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/table/table.go b/table/table.go index c2a5383f3..0565dfeca 100644 --- a/table/table.go +++ b/table/table.go @@ -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. @@ -511,11 +512,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) } From b1e0201c47dd6f49746697e5be6ef24b64848f57 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Wed, 1 Apr 2020 23:28:38 +0530 Subject: [PATCH 2/4] fixup --- table/table.go | 1 + 1 file changed, 1 insertion(+) diff --git a/table/table.go b/table/table.go index 0565dfeca..e7263ae5b 100644 --- a/table/table.go +++ b/table/table.go @@ -382,6 +382,7 @@ func (t *Table) readIndex() error { t.blockIndex = index.Offsets if t.opt.LoadBloomsOnOpen { + // We don't need to acquire lock here because this will be called in sequential manner. t.bf, _ = t.readBloomFilter() } From 7c27b2e22679b55c22ec93c10d89cdb415cd8f09 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Wed, 1 Apr 2020 23:42:10 +0530 Subject: [PATCH 3/4] Add test for race --- table/table_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/table/table_test.go b/table/table_test.go index 46bd94d50..c85891dc1 100644 --- a/table/table_test.go +++ b/table/table_test.go @@ -25,6 +25,7 @@ import ( "os" "sort" "strings" + "sync" "testing" "time" @@ -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() +} From a887aa84cb317d53426f5b69dba8893d2c112f7a Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Mon, 6 Apr 2020 21:41:50 +0530 Subject: [PATCH 4/4] Use lock everywhere --- table/table.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/table/table.go b/table/table.go index e7263ae5b..604150353 100644 --- a/table/table.go +++ b/table/table.go @@ -382,8 +382,9 @@ func (t *Table) readIndex() error { t.blockIndex = index.Offsets if t.opt.LoadBloomsOnOpen { - // We don't need to acquire lock here because this will be called in sequential manner. + t.bfLock.Lock() t.bf, _ = t.readBloomFilter() + t.bfLock.Unlock() } return nil