fix: data race related to last commit info; flush pruning heights as soon as changed #154
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
p0mvn
changed the title
p0mvn
commented
Mar 24, 2022
| @@ -119,15 +124,6 @@ func (m *Manager) ShouldPruneAtHeight(height int64) bool { | |||
| return m.opts.GetPruningStrategy() != types.PruningNothing && m.opts.Interval > 0 && height%int64(m.opts.Interval) == 0 | |||
| } | |||
|
|
|||
| // FlushPruningHeights flushes the pruning heights to the database for crash recovery. | |||
There was a problem hiding this comment.
Moved down and renamed to FlushAllPruningHeights
p0mvn
commented
Mar 24, 2022
| // "All" refers to regular pruning heights and snapshot heights, if any. | ||
| // It serves the same function as exported FlushPruningHeights. However, it assummes that | ||
| // mutex was acquired prior to calling this method. | ||
| func (m *Manager) flushAllPruningHeightsUnlocked() { |
There was a problem hiding this comment.
This is needed because we already flush under lock in HandleHeight
p0mvn
commented
Mar 24, 2022
|
|
||
| rs.mx.RLock() |
There was a problem hiding this comment.
This might be redundant because we only write in the same goroutine above. However, there can be concurrent readers.
What do reviewers think?
p0mvn
commented
Mar 24, 2022
| // If the request's height is the latest height we've committed, then utilize | ||
| // the store's lastCommitInfo as this commit info may not be flushed to disk. | ||
| // Otherwise, we query for the commit info from disk. | ||
| var commitInfo *types.CommitInfo | ||
|
|
There was a problem hiding this comment.
We now flush to disk as soon as rs.lastCommitInfo is updated. Although I acknowledge that db read is more expensive, I think it's safer to make a database read and let the DB handle synchronization. Also, this change makes it more readable imo
p0mvn
commented
Mar 24, 2022
| @@ -1022,33 +1041,16 @@ func getLatestVersion(db dbm.DB) int64 { | |||
| return latestVersion | |||
| } | |||
|
|
|||
| // Gets commitInfo from disk. | |||
14 tasks
p0mvn
changed the title
|
closing in support of #184 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Commit(...)and read inQuery(...)andLastCommitID()rs.lastCommitInfoLeft some extra comments in code
Risks