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.

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: commit info data race #155

Merged
merged 6 commits into from
Mar 26, 2022
Merged

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Mar 25, 2022

Context

  • Fixed data race related to last commit height concurrently written in Commit(...) and read in Query(...) and LastCommitID()
  • flush commit id and version atomically with updating rs.lastCommitInfo
  • WriteSync when flushing metadata since we want it to be immediately written to disk to avoid losing it

Risks

  • This change introduced performance overhead due to synchronization in exchange for safety

@p0mvn p0mvn changed the base branch from master to v0.45.0x-osmo-v7 March 25, 2022 14:31
@@ -1022,36 +1043,17 @@ func getLatestVersion(db dbm.DB) int64 {
return latestVersion
}

// Gets commitInfo from disk.
func getCommitInfo(db dbm.DB, ver int64) (*types.CommitInfo, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Made a method and moved up

@p0mvn p0mvn marked this pull request as ready for review March 25, 2022 15:36
@p0mvn p0mvn requested a review from alexanderbez as a code owner March 25, 2022 15:36
@p0mvn p0mvn requested a review from ValarDragon March 25, 2022 15:36
@p0mvn p0mvn force-pushed the roman/commit-info-data-race branch from 948de1b to 0b829c8 Compare March 25, 2022 18:41
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM!


if err := batch.Write(); err != nil {
if err := batch.WriteSync(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err := batch.WriteSync(); err != nil {
// We use WriteSync to ensure this get written to disk ASAP.
if err := batch.WriteSync(); err != nil {

rs.flushLastCommitInfo(newCommitInfo)
}

func (rs *Store) flushLastCommitInfo(cInfo *types.CommitInfo) {
Copy link
Member

Choose a reason for hiding this comment

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

// should be called with rs.mx.Lock() taken
func (rs *Store) flushLastCommitInfo(cInfo *types.CommitInfo) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants