-
Notifications
You must be signed in to change notification settings - Fork 237
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
Problem: concurrent map access in multistore #1299
Conversation
WalkthroughThe recent changes involve enhancing the thread safety of a Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Signed-off-by: mmsqe <[email protected]>
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- store/rootmulti/store.go (5 hunks)
Additional comments: 6
store/rootmulti/store.go (5)
- 54-54: The addition of the mutex to the
Store
struct is correct and necessary for thread safety.- 68-68: Initialization of the mutex in the
NewStore
function is correct.- 130-132: Proper use of
Lock
andUnlock
in theCommit
method to protect write operations.- 235-236: Correct use of
RLock
andRUnlock
in theGetStore
method to protect read operations.- 242-243: Correct use of
RLock
andRUnlock
in theGetKVStore
method to protect read operations.CHANGELOG.md (1)
- 5-5: The changelog entry is correct and provides the necessary information about the changes made.
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/rootmulti/store.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/rootmulti/store.go
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/rootmulti/store.go (12 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/rootmulti/store.go
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/rootmulti/store.go (11 hunks)
Additional comments: 11
store/rootmulti/store.go (11)
- 54-54: The addition of the mutex to the
Store
struct is correct and necessary for thread safety.- 68-68: The mutex is properly initialized in the
NewStore
function, ensuring eachStore
instance is properly synchronized.- 65-80: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [75-88]
The use of
RLock()
andRUnlock()
in theflush
method is correct, protecting the read operation during iteration over the stores.
- 117-125: The use of
RLock()
andRUnlock()
in theCommit
method is correct for protecting the read operation when iterating over the stores to commit them.- 134-138: The use of
Lock()
andUnlock()
in theCommit
method is correct for protecting the write operation when reloading the stores.- 190-196: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [193-203]
The use of
RLock()
andRUnlock()
in theCacheMultiStore
method is correct for protecting the read operation when copying the stores to the cache.
- 224-230: The use of
RLock()
andRUnlock()
in theCacheMultiStoreWithVersion
method is correct for protecting the read operation when copying the stores to the cache with a specific version.- 242-243: The use of
RLock()
anddefer RUnlock()
in theGetStore
method is correct for protecting the read operation when accessing a specific store.- 249-250: The use of
RLock()
anddefer RUnlock()
in theGetKVStore
method is correct for protecting the read operation when accessing a specific store.- 306-307: The use of
RLock()
anddefer RUnlock()
in theGetCommitKVStore
method is correct for protecting the read operation when accessing a specific store.- 382-384: The use of
Lock()
andUnlock()
in theLoadVersionAndUpgrade
method is correct for protecting the write operation when updating the stores after loading a new version.
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.
LGTM
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/rootmulti/store.go (11 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/rootmulti/store.go
ping on 1.6.1, for more info: cachix/cachix@v1.6.1...v1.7
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .github/workflows/build.yml (1 hunks)
- .github/workflows/test.yml (2 hunks)
Additional comments: 2
.github/workflows/test.yml (1)
- 47-47: The addition of the
installCommand
field in thecachix-action
step is a good practice to ensure that the correct version ofcachix
is installed. However, it's important to verify that the specified version ofcachix
(1.6.1) is compatible with the rest of the workflow and does not introduce any breaking changes..github/workflows/build.yml (1)
- 152-152: The
installCommand
field has been added to thecachix-action
step consistently across different jobs in the workflow. This ensures that thecachix
tool is installed in a controlled manner. As with thetest.yml
file, ensure that the specified version ofcachix
is compatible with the workflow requirements.
store/rootmulti/store.go
Outdated
@@ -63,25 +65,27 @@ | |||
keysByName: make(map[string]types.StoreKey), | |||
stores: make(map[types.StoreKey]types.CommitKVStore), | |||
listeners: make(map[types.StoreKey][]types.WriteListener), | |||
mtx: sync.RWMutex{}, |
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.
Seems not needed
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.
or do we change sync.Map to avoid concurrent map read and map write?
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.
I mean don't need to init explicitly, zero value is the initial value
store/rootmulti/store.go
Outdated
} | ||
} | ||
|
||
// flush writes all the pending change sets to memiavl tree. | ||
func (rs *Store) flush() error { | ||
var changeSets []*memiavl.NamedChangeSet | ||
for key := range rs.stores { | ||
rs.mtx.RLock() |
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.
It's easy to deadlock to grab locks in private method, if it's called by another method which already grabbed locks
store/rootmulti/store.go
Outdated
@@ -361,7 +379,9 @@ |
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.
Why not protecting the other fields? Are the access to them safe?
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.
do you mean storesParams and keysByName, which depends on how we use but we mainly encounter stores concurrent write
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/rootmulti/store.go (11 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/rootmulti/store.go
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1299 +/- ##
===========================================
+ Coverage 15.98% 35.75% +19.77%
===========================================
Files 80 116 +36
Lines 6201 10670 +4469
===========================================
+ Hits 991 3815 +2824
- Misses 5130 6478 +1348
- Partials 80 377 +297 |
@@ -50,6 +51,7 @@ type Store struct { | |||
sdk46Compact bool | |||
// it's more efficient to export snapshot versions, we can filter out the non-snapshot versions | |||
supportExportNonSnapshotVersion bool | |||
mtx sync.RWMutex |
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.
What's the error without this change?
what's the difference with the cosmos-sdk's rootmulti.Store, which don't have a mutex.
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.
I get trace from testnet
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.
I see, the main difference with sdk version is we modified the stores
in Commit()
, in the sdk version, I guess the assumption is the Store
won't be mutated after loaded, that's why it don't need protection, we can try to fix the mutation itself.
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.
#1302
I have a different solution, WDYT?
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make
)make test
)go fmt
)golangci-lint run
)go list -json -m all | nancy sleuth
)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Refactor
Chores
installCommand
parameters.