timshannon / badgerhold

BadgerHold is an embeddable NoSQL store for querying Go types built on Badger
MIT License
515 stars 52 forks source link

Open is unsafe for concurrent use #56

Closed mvdan closed 3 years ago

mvdan commented 3 years ago

I've been digging into a data race caught by go test -race in a downstream project, summarized below:

Write at 0x0000024de218 by goroutine 301:
  github.com/timshannon/badgerhold/v3.Open()
      /home/mvdan/go/pkg/mod/github.com/timshannon/badgerhold/v3@v3.0.0-20210208141506-eb78b03f8097/store.go:56 +0x64
  go.vocdoni.io/dvote/vochain/scrutinizer.InitDB()
      /home/mvdan/src/dvote/vochain/scrutinizer/db.go:77 +0xf7
  [...]

Previous read at 0x0000024de218 by goroutine 142:
  github.com/timshannon/badgerhold/v3.(*Store).TxUpsert()
      /home/mvdan/go/pkg/mod/github.com/timshannon/badgerhold/v3@v3.0.0-20210208141506-eb78b03f8097/put.go:206 +0x3aa
  github.com/timshannon/badgerhold/v3.(*Store).Upsert.func1()
      /home/mvdan/go/pkg/mod/github.com/timshannon/badgerhold/v3@v3.0.0-20210208141506-eb78b03f8097/put.go:168 +0xa4
  [...]

This seems to be because Open sets globals like encode and decode, which are used by methods on Store later on.

This feels wrong - the moment you open a bunch of badgerhold DBs and try to use them, at best you're going to get the wrong behavior (only one wins the write to the global), and at worst you're going to get a panic like the one above.

I think the fix here is simple; the encode and decode variables shouldn't be globals, but rather fields on each Store struct.

I also strongly suggest you to set up a test that creates many DBs in parallel and reads+writes to them, and ensure that go test -race is happy. If you haven't done any concurrency safety testing before, I imagine there will be a few more data races lurking in your code :)

timshannon commented 3 years ago

Yep, you're absolutely right. Bolthold is already written this way, but I haven't made the change over here yet:

https://github.com/timshannon/bolthold/blob/master/store.go

mvdan commented 3 years ago

This was biting us and preventing us from writing parallel tests, so I sent https://github.com/timshannon/badgerhold/pull/58.