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

fix: unprotected read data race #998

Closed
wants to merge 2 commits into from
+10 −1
Closed

fix: unprotected read data race #998

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
646fbcc
fix data race
corverroos Oct 20, 2024
52d81e7
use safeGetLatestVersion
corverroos Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion immutable_tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,15 @@ func (t *ImmutableTree) Get(key []byte) ([]byte, error) {
}

if fastNode == nil {
latestVersion, err := t.ndb.getLatestVersion()
Copy link
Contributor Author

@corverroos corverroos Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note that getLatestVersion() will not return t.ndb.latestVersion if it is 0. I'm not sure if this is a problem or not. This change might therefore result in different logic. We could also add a simple getter that only returns t.ndb.latestVersion directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it is a solution for the above issue. t.ndb.latestVersion should be set within LoadVersion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

to resolve the data race issue, seems like we need to add the API safeGetLatestVersion locked by ndb.mtx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see there is already an exiting method ImmutableTree::isLatestVersion which does what my suggestion does, maybe we should just call that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cool-develope I did the change as you suggested using a new safeGetLatestVersion method instead.

if err != nil {
return nil, err
}

// If the tree is of the latest version and fast node is not in the tree
// then the regular node is not in the tree either because fast node
// represents live state.
if t.version == t.ndb.latestVersion {
if t.version == latestVersion {
return nil, nil
}

Expand Down
Loading