Skip to content

Commit a91b427

Browse files
authored
fix data race with batch reset (#1150)
this is another variation of the race found/fixed in #1092 in that case the batch was empty, which meant we would skip the code that properly synchronized access. our fix only handled this exact case (no data operations), however there is another variation, if the batch contains only deletes (which are data ops) we still spawned the goroutine, although since there were no real updates, again the synchronization code would be skipped, and thus the data race could happen. the fix is to check the number of updates (computed earlier on the caller's goroutine, so it's safe) instead of the length of the IndexOps (which includes updates and deletes) the key is that we should only spawn the goroutine that will range over the batch, in cases where we will synchronize on waiting for the analysis to complete (at least one update). fixes #1149
1 parent 67c3a1f commit a91b427

File tree

3 files changed

+34
-2
lines changed

3 files changed

+34
-2
lines changed

index/scorch/scorch.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ func (s *Scorch) Batch(batch *index.Batch) (err error) {
312312

313313
// FIXME could sort ids list concurrent with analysis?
314314

315-
if len(batch.IndexOps) > 0 {
315+
if numUpdates > 0 {
316316
go func() {
317317
for _, doc := range batch.IndexOps {
318318
if doc != nil {

index/upsidedown/upsidedown.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -810,7 +810,7 @@ func (udc *UpsideDownCouch) Batch(batch *index.Batch) (err error) {
810810
}
811811
}
812812

813-
if len(batch.IndexOps) > 0 {
813+
if numUpdates > 0 {
814814
go func() {
815815
for _, doc := range batch.IndexOps {
816816
if doc != nil {

index_test.go

+32
Original file line numberDiff line numberDiff line change
@@ -2184,3 +2184,35 @@ func TestDataRaceBug1092(t *testing.T) {
21842184
batch.Reset()
21852185
}
21862186
}
2187+
2188+
func TestBatchRaceBug1149(t *testing.T) {
2189+
defer func() {
2190+
err := os.RemoveAll("testidx")
2191+
if err != nil {
2192+
t.Fatal(err)
2193+
}
2194+
}()
2195+
i, err := New("testidx", NewIndexMapping())
2196+
//i, err := NewUsing("testidx", NewIndexMapping(), "scorch", "scorch", nil)
2197+
if err != nil {
2198+
t.Fatal(err)
2199+
}
2200+
defer func() {
2201+
err := i.Close()
2202+
if err != nil {
2203+
t.Fatal(err)
2204+
}
2205+
}()
2206+
b := i.NewBatch()
2207+
b.Delete("1")
2208+
err = i.Batch(b)
2209+
if err != nil {
2210+
t.Fatal(err)
2211+
}
2212+
b.Reset()
2213+
err = i.Batch(b)
2214+
if err != nil {
2215+
t.Fatal(err)
2216+
}
2217+
b.Reset()
2218+
}

0 commit comments

Comments
 (0)