Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Data race in Batch.Reset() #260

Closed
kevgs opened this issue Oct 25, 2015 · 2 comments
Closed

Data race in Batch.Reset() #260

kevgs opened this issue Oct 25, 2015 · 2 comments
Labels
bug

Comments

@kevgs
Copy link

kevgs commented Oct 25, 2015

Here is a code to reproduce an issue.

i, _ := bleve.New("testidx", NewIndexMapping())
b := i.NewBatch()
b.Index("1", 1)
i.Batch(b)
b.Reset()
@mschoch
Copy link
Contributor

mschoch commented Oct 25, 2015

Confirmed, output of race detector:

$ go test -v -run=TestBatchRace -race
=== RUN   TestBatchRace
==================
WARNING: DATA RACE
Write by goroutine 10:
  runtime.mapdelete()
      /Users/mschoch/Documents/research/gosrc/src/runtime/hashmap.go:511 +0x0
  github.com/blevesearch/bleve/index.(*Batch).Reset()
      /Users/mschoch/go/src/github.com/blevesearch/bleve/index/index.go:172 +0x10b
  github.com/blevesearch/bleve.(*Batch).Reset()
      /Users/mschoch/go/src/github.com/blevesearch/bleve/index.go:83 +0x45
  github.com/blevesearch/bleve.TestBatchRace()
      /Users/mschoch/go/src/github.com/blevesearch/bleve/index_test.go:1157 +0x253
  testing.tRunner()
      /Users/mschoch/Documents/research/gosrc/src/testing/testing.go:456 +0xdc

Previous read by goroutine 11:
  runtime.mapiternext()
      /Users/mschoch/Documents/research/gosrc/src/runtime/hashmap.go:621 +0x0
  github.com/blevesearch/bleve/index/upside_down.(*UpsideDownCouch).Batch.func1()
      /Users/mschoch/go/src/github.com/blevesearch/bleve/index/upside_down/upside_down.go:724 +0x2d8

Goroutine 10 (running) created at:
  testing.RunTests()
      /Users/mschoch/Documents/research/gosrc/src/testing/testing.go:561 +0xaa3
  testing.(*M).Run()
      /Users/mschoch/Documents/research/gosrc/src/testing/testing.go:494 +0xe4
  github.com/blevesearch/bleve.TestMain()
      /Users/mschoch/go/src/github.com/blevesearch/bleve/examples_test.go:30 +0xf9
  main.main()
      github.com/blevesearch/bleve/_test/_testmain.go:190 +0x209

Goroutine 11 (finished) created at:
  github.com/blevesearch/bleve/index/upside_down.(*UpsideDownCouch).Batch()
      /Users/mschoch/go/src/github.com/blevesearch/bleve/index/upside_down/upside_down.go:738 +0x25c
  github.com/blevesearch/bleve.(*indexImpl).Batch()
      /Users/mschoch/go/src/github.com/blevesearch/bleve/index_impl.go:310 +0x170
  github.com/blevesearch/bleve.TestBatchRace()
      /Users/mschoch/go/src/github.com/blevesearch/bleve/index_test.go:1156 +0x245
  testing.tRunner()
      /Users/mschoch/Documents/research/gosrc/src/testing/testing.go:456 +0xdc
==================
--- PASS: TestBatchRace (0.00s)
PASS
Found 1 data race(s)
exit status 66
FAIL    github.com/blevesearch/bleve    1.020s

@mschoch mschoch added the bug label Oct 25, 2015
@mschoch mschoch mentioned this issue Mar 20, 2016
Closed
mschoch added a commit to mschoch/bleve that referenced this issue Mar 20, 2016
@mschoch
fix data race in bleve batch reuse
d3cdcbf 
Currently bleve batch is build by user goroutine
Then read by bleve gourinte
This is still safe when used correctly
However, Reset() will modify the map, which is now a data race

This fix is to simply make batch.Reset() alloc new maps.
This provides a data-access pattern that can be used safely.
Also, this thread argues that creating a new map may be faster
than trying to reuse an existing one:

https://groups.google.com/d/msg/golang-nuts/UvUm3LA1u8g/jGv_FobNpN0J

Separate but related, I have opted to remove the "unsafe batch"
checking that we did.  This was always limited anyway, and now
users of Go 1.6 are just as likely to get a panic from the
runtime for concurrent map access anyway.  So, the price paid
by us (additional mutex) is not worth it.

fixes blevesearch#360 and blevesearch#260
@mschoch mschoch mentioned this issue Mar 20, 2016
Closed
mschoch added a commit that referenced this issue Apr 8, 2016
@mschoch
fix data race in bleve batch reuse
b8a2fbb 
Currently bleve batch is build by user goroutine
Then read by bleve gourinte
This is still safe when used correctly
However, Reset() will modify the map, which is now a data race

This fix is to simply make batch.Reset() alloc new maps.
This provides a data-access pattern that can be used safely.
Also, this thread argues that creating a new map may be faster
than trying to reuse an existing one:

https://groups.google.com/d/msg/golang-nuts/UvUm3LA1u8g/jGv_FobNpN0J

Separate but related, I have opted to remove the "unsafe batch"
checking that we did.  This was always limited anyway, and now
users of Go 1.6 are just as likely to get a panic from the
runtime for concurrent map access anyway.  So, the price paid
by us (additional mutex) is not worth it.

fixes #360 and #260
@mschoch
Copy link
Contributor

mschoch commented Apr 8, 2016

This issue was resolved as part of b8a2fbb

@mschoch mschoch closed this as completed Apr 8, 2016
@mschoch mschoch mentioned this issue Feb 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mschoch @kevgs