fix: Make ObjectValue thread safe. #5699
Conversation
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected Products
Test Logs |
Startup Time Report 1Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS. Notes
Startup Times
|
…afeObjectValue # Conflicts: # firebase-firestore/CHANGELOG.md
| boolean modified = false; | ||
|
|
||
| @Nullable Value existingValue = extractNestedValue(partialValue, currentPath); |
There was a problem hiding this comment.
Adding currentValue as parameter as replacement for existingValue, such that this method can be static. This helps reason about side effects, and also eliminates redundant work of extracting existing value from root. The parameter currentValue follows same pattern as currentPath, such that each recursion passes in a child.
This change isn't strictly required for thread safety, but makes the code more efficient and readable.
| @@ -195,7 +214,19 @@ private void setOverlay(FieldPath path, @Nullable Value value) { | |||
| } | |||
| } | |||
|
|
|||
| currentLevel.put(path.getLastSegment(), value); | |||
| String lastSegment = path.getLastSegment(); | |||
There was a problem hiding this comment.
Rather than just putting value every time. We more carefully only put value when there is a change. Otherwise, we will set dirty to true, causing needless work, when values actually were the same.
There was a problem hiding this comment.
@tom-andersen, I have a few questions. But overall the direction looks good to me
firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private static Value extractSegmentValue(Value value, String segment) { | ||
| return Values.isMapValue(value) ? value.getMapValue().getFieldsOrDefault(segment, null) : null; |
There was a problem hiding this comment.
Was it intentional to invert the logic for extracting a value from a MapValue? Previously the code ran Values.isMapValue(value) on the value contained in the input map and returned null if it was not a map. Now that check is not performed on the extracted value. Seems like this could change what this method returns?
There was a problem hiding this comment.
Semantically, this should be equivalent. The helper function is used in other contexts where value might be null, so I had to add check. Since this is being done in helper, it seemed like overkill to also do it in the loop. Technically the loop will work fine even without early return, but the short circuiting behavior has nice performance characteristics.
It isn't the easiest code to read, so any recommendations are welcome.
There was a problem hiding this comment.
I think the new code already is more readable. So thanks for the improvement, and clarifying comment. I don't have any recommendations for improvement, but consider a unit test if you have any concerns about equivalency.
| private static @Nullable MapValue applyOverlay( | ||
| FieldPath currentPath, Map<String, Object> currentOverlays, Value currentValue) { | ||
| if (currentOverlays.isEmpty()) { | ||
| return null; |
There was a problem hiding this comment.
Should this return currentValue?
There was a problem hiding this comment.
No. The method description states:
@return The merged data at `currentPath` or null if no modifications were applied.
This basically just short circuiting logic before more heavy lifting of creating builder objects is done.
There was a problem hiding this comment.
Okay, I see where this is called expects return value of null if there is no overlay. Seems good then, I guess I would just expect a different behavior if the function was being written for the first time.
There was a problem hiding this comment.
These tests look like they may be related to other work
There was a problem hiding this comment.
You are correct. Let me revert that.
| private static @Nullable MapValue applyOverlay( | ||
| FieldPath currentPath, Map<String, Object> currentOverlays, Value currentValue) { | ||
| if (currentOverlays.isEmpty()) { | ||
| return null; |
There was a problem hiding this comment.
Okay, I see where this is called expects return value of null if there is no overlay. Seems good then, I guess I would just expect a different behavior if the function was being written for the first time.
| } | ||
| } | ||
|
|
||
| private static Value extractSegmentValue(Value value, String segment) { | ||
| return Values.isMapValue(value) ? value.getMapValue().getFieldsOrDefault(segment, null) : null; |
There was a problem hiding this comment.
I think the new code already is more readable. So thanks for the improvement, and clarifying comment. I don't have any recommendations for improvement, but consider a unit test if you have any concerns about equivalency.
Internal tracking: b/323342714
There are potential race conditions in ObjectValue.
Past work in response to NullPointerException made access to internal collection more thread safe, but protection is not complete. #3062
Recent support issue has made me suspicious whether thread safety is at fault. #5683
This PR should make thread safety complete. In addition, memoization of proto is improved, thereby reducing contention.