-
Notifications
You must be signed in to change notification settings - Fork 213
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
Conversation
iter, err := txn.Get("main", "foo", key) | ||
assertNilError(t, err) | ||
|
||
for obj := iter.Next(); obj != nil; obj = iter.Next() { |
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.
Without this code change, this test would panic here on iter.Next()
the second iteration.
e260cd8
to
ab383da
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.
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.
ab383da
to
6cb6d9f
Compare
6cb6d9f
to
c26541a
Compare
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:
Closes #85
Closes #86
Closes #88