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

Make ResultIteration safe for use after mutation (option 2) #87

Merged
merged 3 commits into from
Jan 28, 2021

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Jan 22, 2021

This is another option for making the code behave correctly in the scenario described in #85 (other option in #86, #88).

This uses the Txn.Clone added in hashicorp/go-immutable-radix#26 to clone the mutated index before using it in the iterator. From my reading of that PR this use case matches the one it was intended to solve. We need a read-only copy of the yet-to-be-committed tree.

This ensures that the iteration is safe, even if modifications are being made during the iteration. However I believe it does come at the cost of some additional allocations. Once Txn.Clone is called, any further modifications will require creating new nodes rather then re-using the existing modified ones.

TODO:

  • document the expect results of using a ResultIterator in a write transaction

Closes #85
Closes #86
Closes #88

iter, err := txn.Get("main", "foo", key)
assertNilError(t, err)

for obj := iter.Next(); obj != nil; obj = iter.Next() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this code change, this test would panic here on iter.Next() the second iteration.

@dnephin dnephin force-pushed the dnephin/iter-clone-txn branch 2 times, most recently from e260cd8 to ab383da Compare January 26, 2021 19:01
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Code LGTM.

My feeling is that we could be even more super explicit on all the entry points that people querying data are likely to look at (Get mainly but see inline) to make sure they are clear about what consistency is being provided for mutations-while-iterating. See inline comments although there may be other API methods we should add a reference note to that were'nt in your original diff like GetReverse, LowerBound etc.

Approving though as this change seems good to me regardless.

filter.go Show resolved Hide resolved
txn.go Show resolved Hide resolved
txn.go Show resolved Hide resolved
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