-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
fix: Fix race condition in block.incRef #1337
Conversation
This reverts commit 59a4a70.
@@ -197,14 +197,33 @@ func diff(a, b []account) string { | |||
|
|||
var errFailure = errors.New("test failed due to balance mismatch") | |||
|
|||
// get function will fetch the value for the key "k" either by using tht | |||
// txn.Get API or the iterator.Seek API | |||
func get(txn *badger.Txn, k []byte) (*badger.Item, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer if you name this to add random
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ashish-goswami and @jarifibrahim)
table/table.go, line 196 at r1 (raw file):
ref := atomic.LoadInt32(&b.ref) // The ref would not be equal to 0 unless the existing // block get evicted before this line. If the ref is 0. it means that
If the ref is zero, it means
Do mention that we can't just blindly add 1 to ref. We need to check whether it has reached zero first, because if it did, then we should absolutely not use this block.
table/table.go, line 451 at r1 (raw file):
blk, ok := t.opt.Cache.Get(key) if ok && blk != nil { b := blk.(*block)
if b := ..; b.incrRef() { return b }
All review comments have been addressed. |
This reverts commit 21735af.
This reverts commit 21735af.
This reverts commit 21735af.
This reverts commit 21735af. The commit is reverted because we have seen some crashes which could be caused by this change. We haven't been able to reproduce the crashes yet. Related to #1389, #1388, #1387 Also, see https://discuss.dgraph.io/t/current-state-of-badger-crashes/7602
Also, disable conflict detection in badger to save memory. ``` 0dfb8b4 Changelog for v20.07.0 (dgraph-io/badger#1411) 03ba278 Add missing changelog for v2.0.3 (dgraph-io/badger#1410) 6001230 Revert "Compress/Encrypt Blocks in the background (dgraph-io/badger#1227)" (dgraph-io/badger#1409) 800305e Revert "Buffer pool for decompression (dgraph-io/badger#1308)" (dgraph-io/badger#1408) 63d9309 Revert "fix: Fix race condition in block.incRef (dgraph-io/badger#1337)" (dgraph-io/badger#1407) e0d058c Revert "add assert to check integer overflow for table size (dgraph-io/badger#1402)" (dgraph-io/badger#1406) d981f47 return error if the vlog writes exceeds more that 4GB. (dgraph-io/badger#1400) 7f4e4b5 add assert to check integer overflow for table size (dgraph-io/badger#1402) 8e896a7 Add a contribution guide (dgraph-io/badger#1379) b79aeef Avoid panic on multiple closer.Signal calls (dgraph-io/badger#1401) 717b89c Enable cross-compiled 32bit tests on TravisCI (dgraph-io/badger#1392) 09dfa66 Update ristretto to commit f66de99 (dgraph-io/badger#1391) 509de73 Update head while replaying value log (dgraph-io/badger#1372) e013bfd Rework DB.DropPrefix (dgraph-io/badger#1381) 3042e37 pre allocate cache key for the block cache and the bloom filter cache (dgraph-io/badger#1371) 675efcd Increase default valueThreshold from 32B to 1KB (dgraph-io/badger#1346) 158d927 Remove second initialization of writech in Open (dgraph-io/badger#1382) d37ce36 Tests: Use t.Parallel in TestIteratePrefix tests (dgraph-io/badger#1377) 3f4761d Force KeepL0InMemory to be true when InMemory is true (dgraph-io/badger#1375) dd332b0 Avoid panic in filltables() (dgraph-io/badger#1365) c45d966 Fix assert in background compression and encryption. (dgraph-io/badger#1366) ```
This commit brings the following new changes from badger. This commit also disable conflict detection in badger to save memory. ``` Fix assert in background compression and encryption. (dgraph-io/badger#1366) Avoid panic in filltables() (dgraph-io/badger#1365) Force KeepL0InMemory to be true when InMemory is true (dgraph-io/badger#1375) Tests: Use t.Parallel in TestIteratePrefix tests (dgraph-io/badger#1377) Remove second initialization of writech in Open (dgraph-io/badger#1382) Increase default valueThreshold from 32B to 1KB (dgraph-io/badger#1346) pre allocate cache key for the block cache and the bloom filter cache (dgraph-io/badger#1371) Rework DB.DropPrefix (dgraph-io/badger#1381) Update head while replaying value log (dgraph-io/badger#1372) Update ristretto to commit f66de99 (dgraph-io/badger#1391) Enable cross-compiled 32bit tests on TravisCI (dgraph-io/badger#1392) Avoid panic on multiple closer.Signal calls (dgraph-io/badger#1401) Add a contribution guide (dgraph-io/badger#1379) add assert to check integer overflow for table size (dgraph-io/badger#1402) return error if the vlog writes exceeds more that 4GB. (dgraph-io/badger#1400) Revert "add assert to check integer overflow for table size (dgraph-io/badger#1402)" (dgraph-io/badger#1406) Revert "fix: Fix race condition in block.incRef (dgraph-io/badger#1337)" (dgraph-io/badger#1407) Revert "Buffer pool for decompression (dgraph-io/badger#1308)" (dgraph-io/badger#1408) Revert "Compress/Encrypt Blocks in the background (dgraph-io/badger#1227)" (dgraph-io/badger#1409) Add missing changelog for v2.0.3 (dgraph-io/badger#1410) Changelog for v20.07.0 (dgraph-io/badger#1411) ```
This commit brings following new changes from badger This commit also disable conflict detection in badger to save memory. ``` Fix assert in background compression and encryption. (dgraph-io/badger#1366) Avoid panic in filltables() (dgraph-io/badger#1365) Force KeepL0InMemory to be true when InMemory is true (dgraph-io/badger#1375) Tests: Use t.Parallel in TestIteratePrefix tests (dgraph-io/badger#1377) Remove second initialization of writech in Open (dgraph-io/badger#1382) Increase default valueThreshold from 32B to 1KB (dgraph-io/badger#1346) pre allocate cache key for the block cache and the bloom filter cache (dgraph-io/badger#1371) Rework DB.DropPrefix (dgraph-io/badger#1381) Update head while replaying value log (dgraph-io/badger#1372) Update ristretto to commit f66de99 (dgraph-io/badger#1391) Enable cross-compiled 32bit tests on TravisCI (dgraph-io/badger#1392) Avoid panic on multiple closer.Signal calls (dgraph-io/badger#1401) Add a contribution guide (dgraph-io/badger#1379) add assert to check integer overflow for table size (dgraph-io/badger#1402) return error if the vlog writes exceeds more that 4GB. (dgraph-io/badger#1400) Revert "add assert to check integer overflow for table size (dgraph-io/badger#1402)" (dgraph-io/badger#1406) Revert "fix: Fix race condition in block.incRef (dgraph-io/badger#1337)" (dgraph-io/badger#1407) Revert "Buffer pool for decompression (dgraph-io/badger#1308)" (dgraph-io/badger#1408) Revert "Compress/Encrypt Blocks in the background (dgraph-io/badger#1227)" (dgraph-io/badger#1409) Add missing changelog for v2.0.3 (dgraph-io/badger#1410) Changelog for v20.07.0 (dgraph-io/badger#1411) ```
This commit brings following new changes from badger This commit also disable conflict detection in badger to save memory. ``` Fix assert in background compression and encryption. (dgraph-io/badger#1366) Avoid panic in filltables() (dgraph-io/badger#1365) Force KeepL0InMemory to be true when InMemory is true (dgraph-io/badger#1375) Tests: Use t.Parallel in TestIteratePrefix tests (dgraph-io/badger#1377) Remove second initialization of writech in Open (dgraph-io/badger#1382) Increase default valueThreshold from 32B to 1KB (dgraph-io/badger#1346) pre allocate cache key for the block cache and the bloom filter cache (dgraph-io/badger#1371) Rework DB.DropPrefix (dgraph-io/badger#1381) Update head while replaying value log (dgraph-io/badger#1372) Update ristretto to commit f66de99 (dgraph-io/badger#1391) Enable cross-compiled 32bit tests on TravisCI (dgraph-io/badger#1392) Avoid panic on multiple closer.Signal calls (dgraph-io/badger#1401) Add a contribution guide (dgraph-io/badger#1379) add assert to check integer overflow for table size (dgraph-io/badger#1402) return error if the vlog writes exceeds more that 4GB. (dgraph-io/badger#1400) Revert "add assert to check integer overflow for table size (dgraph-io/badger#1402)" (dgraph-io/badger#1406) Revert "fix: Fix race condition in block.incRef (dgraph-io/badger#1337)" (dgraph-io/badger#1407) Revert "Buffer pool for decompression (dgraph-io/badger#1308)" (dgraph-io/badger#1408) Revert "Compress/Encrypt Blocks in the background (dgraph-io/badger#1227)" (dgraph-io/badger#1409) Add missing changelog for v2.0.3 (dgraph-io/badger#1410) Changelog for v20.07.0 (dgraph-io/badger#1411) ```
This commit brings following new changes from badger This commit also disable conflict detection in badger to save memory. ``` Fix assert in background compression and encryption. (dgraph-io/badger#1366) Avoid panic in filltables() (dgraph-io/badger#1365) Force KeepL0InMemory to be true when InMemory is true (dgraph-io/badger#1375) Tests: Use t.Parallel in TestIteratePrefix tests (dgraph-io/badger#1377) Remove second initialization of writech in Open (dgraph-io/badger#1382) Increase default valueThreshold from 32B to 1KB (dgraph-io/badger#1346) pre allocate cache key for the block cache and the bloom filter cache (dgraph-io/badger#1371) Rework DB.DropPrefix (dgraph-io/badger#1381) Update head while replaying value log (dgraph-io/badger#1372) Update ristretto to commit f66de99 (dgraph-io/badger#1391) Enable cross-compiled 32bit tests on TravisCI (dgraph-io/badger#1392) Avoid panic on multiple closer.Signal calls (dgraph-io/badger#1401) Add a contribution guide (dgraph-io/badger#1379) add assert to check integer overflow for table size (dgraph-io/badger#1402) return error if the vlog writes exceeds more that 4GB. (dgraph-io/badger#1400) Revert "add assert to check integer overflow for table size (dgraph-io/badger#1402)" (dgraph-io/badger#1406) Revert "fix: Fix race condition in block.incRef (dgraph-io/badger#1337)" (dgraph-io/badger#1407) Revert "Buffer pool for decompression (dgraph-io/badger#1308)" (dgraph-io/badger#1408) Revert "Compress/Encrypt Blocks in the background (dgraph-io/badger#1227)" (dgraph-io/badger#1409) Add missing changelog for v2.0.3 (dgraph-io/badger#1410) Changelog for v20.07.0 (dgraph-io/badger#1411) ```
* Update badger to v20.07.0-rc1 This commit brings following new changes from badger This commit also disable conflict detection in badger to save memory. ``` Fix assert in background compression and encryption. (dgraph-io/badger#1366) Avoid panic in filltables() (dgraph-io/badger#1365) Force KeepL0InMemory to be true when InMemory is true (dgraph-io/badger#1375) Tests: Use t.Parallel in TestIteratePrefix tests (dgraph-io/badger#1377) Remove second initialization of writech in Open (dgraph-io/badger#1382) Increase default valueThreshold from 32B to 1KB (dgraph-io/badger#1346) pre allocate cache key for the block cache and the bloom filter cache (dgraph-io/badger#1371) Rework DB.DropPrefix (dgraph-io/badger#1381) Update head while replaying value log (dgraph-io/badger#1372) Update ristretto to commit f66de99 (dgraph-io/badger#1391) Enable cross-compiled 32bit tests on TravisCI (dgraph-io/badger#1392) Avoid panic on multiple closer.Signal calls (dgraph-io/badger#1401) Add a contribution guide (dgraph-io/badger#1379) add assert to check integer overflow for table size (dgraph-io/badger#1402) return error if the vlog writes exceeds more that 4GB. (dgraph-io/badger#1400) Revert "add assert to check integer overflow for table size (dgraph-io/badger#1402)" (dgraph-io/badger#1406) Revert "fix: Fix race condition in block.incRef (dgraph-io/badger#1337)" (dgraph-io/badger#1407) Revert "Buffer pool for decompression (dgraph-io/badger#1308)" (dgraph-io/badger#1408) Revert "Compress/Encrypt Blocks in the background (dgraph-io/badger#1227)" (dgraph-io/badger#1409) Add missing changelog for v2.0.3 (dgraph-io/badger#1410) Changelog for v20.07.0 (dgraph-io/badger#1411) ``` * Remove unnecessary detect conflicts
* Update badger to v20.07.0-rc1 This commit brings the following new changes from badger. This commit also disable conflict detection in badger to save memory. ``` Fix assert in background compression and encryption. (dgraph-io/badger#1366) Avoid panic in filltables() (dgraph-io/badger#1365) Force KeepL0InMemory to be true when InMemory is true (dgraph-io/badger#1375) Tests: Use t.Parallel in TestIteratePrefix tests (dgraph-io/badger#1377) Remove second initialization of writech in Open (dgraph-io/badger#1382) Increase default valueThreshold from 32B to 1KB (dgraph-io/badger#1346) pre allocate cache key for the block cache and the bloom filter cache (dgraph-io/badger#1371) Rework DB.DropPrefix (dgraph-io/badger#1381) Update head while replaying value log (dgraph-io/badger#1372) Update ristretto to commit f66de99 (dgraph-io/badger#1391) Enable cross-compiled 32bit tests on TravisCI (dgraph-io/badger#1392) Avoid panic on multiple closer.Signal calls (dgraph-io/badger#1401) Add a contribution guide (dgraph-io/badger#1379) add assert to check integer overflow for table size (dgraph-io/badger#1402) return error if the vlog writes exceeds more that 4GB. (dgraph-io/badger#1400) Revert "add assert to check integer overflow for table size (dgraph-io/badger#1402)" (dgraph-io/badger#1406) Revert "fix: Fix race condition in block.incRef (dgraph-io/badger#1337)" (dgraph-io/badger#1407) Revert "Buffer pool for decompression (dgraph-io/badger#1308)" (dgraph-io/badger#1408) Revert "Compress/Encrypt Blocks in the background (dgraph-io/badger#1227)" (dgraph-io/badger#1409) Add missing changelog for v2.0.3 (dgraph-io/badger#1410) Changelog for v20.07.0 (dgraph-io/badger#1411) ``` * Remove unnecessary detect conflicts
* Update badger to v20.07.0-rc1 This commit brings following new changes from badger This commit also disable conflict detection in badger to save memory. ``` Fix assert in background compression and encryption. (dgraph-io/badger#1366) Avoid panic in filltables() (dgraph-io/badger#1365) Force KeepL0InMemory to be true when InMemory is true (dgraph-io/badger#1375) Tests: Use t.Parallel in TestIteratePrefix tests (dgraph-io/badger#1377) Remove second initialization of writech in Open (dgraph-io/badger#1382) Increase default valueThreshold from 32B to 1KB (dgraph-io/badger#1346) pre allocate cache key for the block cache and the bloom filter cache (dgraph-io/badger#1371) Rework DB.DropPrefix (dgraph-io/badger#1381) Update head while replaying value log (dgraph-io/badger#1372) Update ristretto to commit f66de99 (dgraph-io/badger#1391) Enable cross-compiled 32bit tests on TravisCI (dgraph-io/badger#1392) Avoid panic on multiple closer.Signal calls (dgraph-io/badger#1401) Add a contribution guide (dgraph-io/badger#1379) add assert to check integer overflow for table size (dgraph-io/badger#1402) return error if the vlog writes exceeds more that 4GB. (dgraph-io/badger#1400) Revert "add assert to check integer overflow for table size (dgraph-io/badger#1402)" (dgraph-io/badger#1406) Revert "fix: Fix race condition in block.incRef (dgraph-io/badger#1337)" (dgraph-io/badger#1407) Revert "Buffer pool for decompression (dgraph-io/badger#1308)" (dgraph-io/badger#1408) Revert "Compress/Encrypt Blocks in the background (dgraph-io/badger#1227)" (dgraph-io/badger#1409) Add missing changelog for v2.0.3 (dgraph-io/badger#1410) Changelog for v20.07.0 (dgraph-io/badger#1411) ``` * Remove unnecessary detect conflicts Co-authored-by: parasssh <paras@dgraph.io>
* Update badger to v20.0.7-rc1 This commit brings following new changes from badger This commit also disable conflict detection in badger to save memory. ``` Fix assert in background compression and encryption. (dgraph-io/badger#1366) Avoid panic in filltables() (dgraph-io/badger#1365) Force KeepL0InMemory to be true when InMemory is true (dgraph-io/badger#1375) Tests: Use t.Parallel in TestIteratePrefix tests (dgraph-io/badger#1377) Remove second initialization of writech in Open (dgraph-io/badger#1382) Increase default valueThreshold from 32B to 1KB (dgraph-io/badger#1346) pre allocate cache key for the block cache and the bloom filter cache (dgraph-io/badger#1371) Rework DB.DropPrefix (dgraph-io/badger#1381) Update head while replaying value log (dgraph-io/badger#1372) Update ristretto to commit f66de99 (dgraph-io/badger#1391) Enable cross-compiled 32bit tests on TravisCI (dgraph-io/badger#1392) Avoid panic on multiple closer.Signal calls (dgraph-io/badger#1401) Add a contribution guide (dgraph-io/badger#1379) add assert to check integer overflow for table size (dgraph-io/badger#1402) return error if the vlog writes exceeds more that 4GB. (dgraph-io/badger#1400) Revert "add assert to check integer overflow for table size (dgraph-io/badger#1402)" (dgraph-io/badger#1406) Revert "fix: Fix race condition in block.incRef (dgraph-io/badger#1337)" (dgraph-io/badger#1407) Revert "Buffer pool for decompression (dgraph-io/badger#1308)" (dgraph-io/badger#1408) Revert "Compress/Encrypt Blocks in the background (dgraph-io/badger#1227)" (dgraph-io/badger#1409) Add missing changelog for v2.0.3 (dgraph-io/badger#1410) Changelog for v20.07.0 (dgraph-io/badger#1411) ``` * Remove unnecessary detect conflicts Co-authored-by: parasssh <paras@dgraph.io>
* Update badger to v20.07.0-rc1 This commit brings the following new changes from badger. This commit also disable conflict detection in badger to save memory. ``` Fix assert in background compression and encryption. (dgraph-io/badger#1366) Avoid panic in filltables() (dgraph-io/badger#1365) Force KeepL0InMemory to be true when InMemory is true (dgraph-io/badger#1375) Tests: Use t.Parallel in TestIteratePrefix tests (dgraph-io/badger#1377) Remove second initialization of writech in Open (dgraph-io/badger#1382) Increase default valueThreshold from 32B to 1KB (dgraph-io/badger#1346) pre allocate cache key for the block cache and the bloom filter cache (dgraph-io/badger#1371) Rework DB.DropPrefix (dgraph-io/badger#1381) Update head while replaying value log (dgraph-io/badger#1372) Update ristretto to commit f66de99 (dgraph-io/badger#1391) Enable cross-compiled 32bit tests on TravisCI (dgraph-io/badger#1392) Avoid panic on multiple closer.Signal calls (dgraph-io/badger#1401) Add a contribution guide (dgraph-io/badger#1379) add assert to check integer overflow for table size (dgraph-io/badger#1402) return error if the vlog writes exceeds more that 4GB. (dgraph-io/badger#1400) Revert "add assert to check integer overflow for table size (dgraph-io/badger#1402)" (dgraph-io/badger#1406) Revert "fix: Fix race condition in block.incRef (dgraph-io/badger#1337)" (dgraph-io/badger#1407) Revert "Buffer pool for decompression (dgraph-io/badger#1308)" (dgraph-io/badger#1408) Revert "Compress/Encrypt Blocks in the background (dgraph-io/badger#1227)" (dgraph-io/badger#1409) Add missing changelog for v2.0.3 (dgraph-io/badger#1410) Changelog for v20.07.0 (dgraph-io/badger#1411) ``` * Remove unnecessary detect conflicts
Fixes hypermodeinc/dgraph#5456 . This PR fixes the crash that could occur when a block was read from the cache. There was a logical race condition. The following sequence of events could occur which would cause the crash. 1. An iterator makes `t.Block(idx)` call 2. The `t.Block` function finds the block in the cache. The newly found block has `ref=1` which means it was held only by the cache. 3. The `t.Block` function is holding the block and at the same time the block gets evicted from the cache. The existing ref of the block was `1` so the cache eviction would decrement the ref and make it `0`. When the ref becomes `0`, the block is added to the `sync.Pool` and is ready to be reused. 4. While the block got evicted from the cache, the iterator received the block and it incremented the ref from `0` to `1` and starts using this. Since the block was inserted into the syncPool in the 3rd event, it could be modified by anyone while the iterator is using it.
This reverts commit 21735af. The commit is reverted because we have seen some crashes which could be caused by this change. We haven't been able to reproduce the crashes yet. Related to #1389, #1388, #1387 Also, see https://discuss.dgraph.io/t/current-state-of-badger-crashes/7602
Fixes hypermodeinc/dgraph#5456 .
This PR fixes the crash that could occur when a block was read from the cache.
There was a logical race condition. The following sequence of events could occur which would cause the crash.
t.Block(idx)
callt.Block
function finds the block in the cache. The newly found block hasref=1
which means it was held only by the cache.t.Block
function is holding the block and at the same time the block gets evicted from the cache. The existing ref of the block was1
so the cache eviction would decrement the ref and make it0
. When the ref becomes0
, the block is added to thesync.Pool
and is ready to be reused.0
to1
and starts using this.Since the block was inserted into the syncPool in the 3rd event, it could be modified by anyone while the iterator is using it.
The fix is to not increment the block ref if it is already zero.
This change is