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

fix bug fatal error: concurrent map read and map write #23

Merged
merged 1 commit into from
Feb 21, 2022
Merged

fix bug fatal error: concurrent map read and map write #23

merged 1 commit into from
Feb 21, 2022

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Feb 21, 2022

Background

Creating a block explorer on archive node seems to be causing fatal errors: #22

The logs indicate that there was a grpc query to the SDK that got propagated to IAVL while in a separate goroutine the ndoe was committing:

https://gist.github.com/rkben/8814376162cda8c08dd33d60f6933429#file-cleand_osmo-log-L1257 - this goroutine seems to be committing a block
https://gist.github.com/rkben/8814376162cda8c08dd33d60f6933429#file-cleand_osmo-log-L268 - this goroutine seems to be handling the grpc query

Initial investigation points at the fact that fastNodeCache is not being synchronized correctly.

In this PR

  • ensured that fastNodeCache cache is only accessed from within nodedb
  • access to fastNodeCache in nodedb is synchronized
  • comments added, reminding about the need for synchrony
  • all tests pass and no deadlock
  • created a ticket to refactor synchrony:

ValarDragon
ValarDragon reviewed Feb 21, 2022
View reviewed changes
nodedb.go
Comment on lines +154 to +155
ndb.mtx.Lock()
defer ndb.mtx.Unlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a RLock?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait why did moving this before hasUpgradedToFastStorage have any impact?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

storageVersion is still a common resource that might be written and read at the same time

In addition, for readability. Easier to see locks at the top of a method

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking into RLock...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we are modifying a common resource. As a result, I don't think RLock could work

ValarDragon
ValarDragon approved these changes Feb 21, 2022
View reviewed changes
Copy link
Member

@ValarDragon ValarDragon left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this patch so quickly for the failing explorer

I think that:

  • The GRPC query is doing the thing thats wrong
  • We should come back and revisit the mutex contentions. (If we need mutex contentions, lets get the I/O operation not under mutex, and only RAM updates for caches)

@p0mvn
Copy link
Member Author

p0mvn commented Feb 21, 2022

@rohanspersonalgit FYI about mutex contentions in #24

  • We should come back and revisit the mutex contentions. (If we need mutex contentions, lets get the I/O operation not under mutex, and only RAM updates for caches)

@p0mvn p0mvn merged commit 2a1daf4 into dev/iavl_data_locality Feb 21, 2022
p0mvn added a commit that referenced this pull request Feb 28, 2022
@p0mvn
sync access to fast node cache to avoid concurrent write fatal error (#…
1236c4c 
…23)
p0mvn added a commit that referenced this pull request Mar 2, 2022
@p0mvn
Revert "sync access to fast node cache to avoid concurrent write fata…
fe90684 
…l error (#23)"

This reverts commit 2a1daf4.
@p0mvn p0mvn mentioned this pull request Mar 3, 2022
Merged
p0mvn added a commit that referenced this pull request Mar 3, 2022
@p0mvn
revert #23 (sync access to fast node cache), fix bug related to old h…
2e0b26a 
…eight export (#33)

* Revert "sync access to fast node cache to avoid concurrent write fatal error (#23)"

This reverts commit 2a1daf4.

* return correct iterator in mutable tree
p0mvn added a commit that referenced this pull request Mar 28, 2022
@p0mvn
revert #23 (sync access to fast node cache), fix bug related to old h…
67b66c0 
…eight export (#33)

* Revert "sync access to fast node cache to avoid concurrent write fatal error (#23)"

This reverts commit 2a1daf4.

* return correct iterator in mutable tree
faddat pushed a commit to faddat/iavl that referenced this pull request Apr 11, 2022
@p0mvn @ValarDragon @jtieri
feat: Fast storage optimization for queries and iterations (cosmos#468)
0dcb21b 
* Add unbounded key string to KeyFormat

* Add test vectors for unbounded length keys

* Add some notes

* update .gitignore

* Add FastNode struct

* WIP: make Get work with new FastNode

* when retrieving fastnode fails, return errors vs. panic

* add comments clarifying what index represents

* make the linter happy

* Add small tweaks to fast_node.go

* add TODO & small linter tweaks

* Update comment

* update fast node cache in set

* add debugging output when falling back to original logic

* return error instead of panic

* WIP: refactor set ops to work with fast store

* update Set of mutable tree, begin unit testing

* update GetVersioned to check fast nodes before trying the immutable

* check fast node version before nil value check in get of immutable tree

* fix small bugs and typos, continue writing unit tests for Set

* unit test saveFastNodeVersion

* simplify storing unsaved fast nodes

* resolve a bug with not writing prefix for fast node to disk

* remove fast nodes from disk on save and clear fast cache when version is deleted, fix all tests but random and with index

* resolve an issue with randomized tests caused by the fast node cache not being cleared when latest version is saved

* split unsaved fast node changes into additions and removals

* save fast node removals

* move fast node cache clearing to nodedb

* use regular logic only when fast node version is greater than immutable tree'

* clean up tree_random_test.go

* clear unsaved fast node removals on rollback

* fix randomized test failures caused by a typo in ndb DeleteVersion for loop

* implement GetFast method to preserve Get with correct index return, convert unit tests from Get to GetFast where applicable

* ensure Get and GetFast return the same values in tree_random_test.go

* test fast node cache is live in random tree tests

* improve mutable tree unit tests related to new functionality

* clean up tree_test.go

* implement GetVersionedFast to preserve the index in GetVersioned

* restore accidentally deleted test in mutable tree test

* spurious whitespace

* refactor mutable tree

* fix comment in mutable tree

* add benchmark results

* avoid redundant full tree search in GetFast of immutable tree when fast node is nil and tree is latest

* fix naming for bench test get on random keys

* use method for get latestversio in get fast

* optimize GetFast, perform a refactor to ensure that fast nodes on disk are matched and better test

* add latest bench

* Fast Node Iteration (osmosis-labs#7)

* propagate errors from nodedb and avoid panicking

* begin implementing fast node iteration

* resolve rebase issue in random tests

* fix iteration to deserialize fast node for extracting the correct value

* finalzie iteration and let all unit tests pass

* add benchmarks

* merge GetVersioned and GetVersionedFast

* remove fast node deletion from DeleteVersion and DeleteVersionRange and benchmark

* fix and unit test iteration on mutable and immutable trees

* implement tests for iterator and iterate, begin testing fast iterator

* fix and unit test fast iterator

* refactor iterate methods of mutable and immutable trees

* resolve some warnings

* remove old bench results

* refactor bench tests for iteration

* Fast Cache Migration (osmosis-labs#9)

* implement nodedb changes to set and get chain version from the database

* implement and unit test upgrade to fast storage in mutable tree

* refactor for auto upgrade to fast version in mutable tree contructor, load version and lazy load version

* use proper functionality for getting latest version

* remove unused error

* fix comment

* use fast version value in tests

* spurious tab

* fix style problems and remove redundant code in tests

* Rename Get to GetWithIndex and GetFast to Get

* refactor and clean up bench tests

* remove extra byte from fast node length

* clean up immutable tree

* refactor nil tree or ndb error for the iterators and their tests

* avoid exporting methods for getting unsaved additions and removals

* refactor fast upgrade to read from tree instead of traversing disk nodes and orphans, update unit tests

* remove unneeded comment

* refer to storage version consistently across the project

* fix more warnings

* optimize removal of fast nodes from cache on deletion

* small changes in teh mutable tree

* correct storage version key

* auto set fast version in SaveVersion

* avoid exporting new methods of the immutable tree

* go fmt

* Fix comment in fast iterator

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* add extra comment for domain in fast iterator

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* add comments for moving the iterator before the first element

* add comment for describing what mirror is in assertIterator of testutils

* fix checking the mirror for descending iterator in tests

* Update testutils_test.go with a comment

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* Update benchmarks/bench_test.go with a comment for runKnownQueriesFast

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* Update benchmarks/bench_test.go with a comment for runQueriesFast

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* export IsFastCacheEnabled and add an assert in bench tests

* Update comment immutable_tree.go

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* Update comment for migration in mutable_tree.go

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* simlify Iterate in mutable tree, add debug log for

* Fast Cache - Downgrade - reupgrade protection and other improvements (osmosis-labs#12)

* add leaf hash to fast node and unit test

* refactor get with index and get by index, fix migration in load version and lazy load version

* use Get in GetVersioned of mutable tree

* refactor non membership proof to use fast storage if available

* bench non-membership proof

* fix bench tests to work with the new changes

* add downgrade-reupgrade protection and unit test

* remove leaf hash from fast node

* resolve multithreading bug related to iterators not being closed

* clean up

* use correct tree in bench tests

* add cache to tree used to bench non membership proofs

* add benc tests for GetWithIndex and GetByIndex

* revert GetWithIndex and GetByIndex

* remove unused import

* unit test re-upgrade protection and fix small issues

* remove redundant setStorageVersion method

* fix bug with appending to live stage version to storage version and nit test

* add comment for setFastStorageVersionToBatch

* refactor and improve unit tests for reupgrade protection

* rename ndb's isFastStorageEnabled to hasUpgradedToFastStorage and add comments

* comment out new implementation for GetNonMembershipProof

* update comments in nodedb to reflect the difference between hasUpgradedToFastStorage and shouldForceFastStorageUpdate

* refactor nodedb tests

* downgrade tendermint to 0.34.14 - osmosis's latest cosmos sdk does not support 0.35.0

* fix bug where fast storage was not enabled when version 0 was attempted to be loaded

* implement unsaved fast iterator to be used in mutable tree (osmosis-labs#16)

* address comments from unsaved fast iterator PR

* expose isUpgradeable method on mutable tree and unit test (osmosis-labs#17)

* expose isUpgradeable method on mutable tree and unit test

* go fmt

* resolve problems with rebasing

* update CHANGELOG.md

* tendermint v0.35.0

* fix String() bench

* fix duplication lint in iterator_test.go

* fix lint for tree.ndb.DeleteFastNode error return not checked

* fix Error return value of `ndb.resetBatch` is not checked

* fix Error return value of `ndb.traversePrefix` is not checked

* fix Error: struct of size 64 bytes could be of size 56 bytes

* fix Error: `comitted` is a misspelling of `committed`

* fix issues in basic_test.go

* address comments in fast_iterator.go

* address comments in immutable tree

* address comments in iterator.go

* address comments in key_format.go

* address remaining comments

* fix Error: Error return value of `ndb.batch.Write` is not checked

* fix Error: receiver name t should be consistent with previous receiver name tree for MutableTree

* fix Error: struct of size 48 bytes could be of size 40 bytes

* go fmt

* more linter fixes

* fix remaining linter problems

* upgrade tm-db and comment out broken bencher databases

* skip iterations for BenchmarkLevelDBLargeData bench

* attempt to fix linter

* force GC, no cache during migration, auto heap profile (osmosis-labs#19)

* force GC, no cache during migration, auto heap profile

* resolve a potential deadlock from racing between reset and stop

* fix small lint issue

* remove logs and pprof logic

* remove unused libraries

* add comment explaining the reason for RAM optimizations

* sync access to fast node cache to avoid concurrent write fatal error (osmosis-labs#23)

* update go.mod and go.sum to match master versions

* revert osmosis-labs#23 (sync access to fast node cache), fix bug related to old height export (osmosis-labs#33)

* Revert "sync access to fast node cache to avoid concurrent write fatal error (osmosis-labs#23)"

This reverts commit 2a1daf4.

* return correct iterator in mutable tree

* fix concurrent map panic when querying and comittting concurrently

* avoid clearing fast node cache during pruning (osmosis-labs#35)

* fix data race related to VersionExists (osmosis-labs#36)

* fix data race related to VersionExists

* use regular lock instead of RW in mutable_tree.go

* hardcode fast node cache size to  100k

* go fmt

* restore proof_ics23.go

* fix linter

Co-authored-by: ValarDragon <dojha12@gmail.com>
Co-authored-by: jtieri <37750742+jtieri@users.noreply.github.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Roman Akhtariev <34196718+akhtariev@users.noreply.github.com>
Co-authored-by: Roman <34196718+r0mvn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValarDragon ValarDragon ValarDragon approved these changes

@czarcas7ic czarcas7ic Awaiting requested review from czarcas7ic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@p0mvn @ValarDragon