-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(baseapp): introduce mutex to state #18846
Conversation
WalkthroughThe changes across the files aim to enhance thread safety in the Cosmos SDK by introducing mutexes and updating the context handling. The modifications include encapsulation of context within state structures, ensuring synchronized access to shared resources, and altering method signatures to accommodate the new thread-safe design. These changes aim to address race conditions, specifically within the Changes
Assessment against linked issues
The code changes appear to align with the objectives of addressing thread safety concerns, as indicated in the linked issue. However, without explicit mention of the 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 X ? TipsChat with CodeRabbit Bot (
|
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.
I'm not 100% sure that state objects themselves are being updated/read in a threadsafe way (would have to check), but this solves the problem for context access.
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.
utACK. this is for sure safer than what we have today. I wonder if there will be any sort of regressions in performance, but its mute since this is safer. With server v2 we will be able to avoid this
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, can you fix the lint issue coming from imports in state.go file?
c75442b
to
bcf2d8a
Compare
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. Just two minor nits
Co-authored-by: Aleksandr Bezobchuk <[email protected]> (cherry picked from commit c519104) # Conflicts: # baseapp/abci.go
Co-authored-by: Nikhil Vasan <[email protected]> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Description
Closes: #18844
In This PR
sync.RWMutex
to thestate
objectReadLock
on each call toContext
(getter)Lock
on each call toUpdateContext
(setter).ctx
accesses to eitherContext()
for reads, orUpdateContext
for writesAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...