Skip to content

fix: Make ObjectValue thread safe. #5699

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

fix: Make ObjectValue thread safe. #5699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
54f0766
fix: Make ObjectValue thread safe.
tom-andersen Feb 8, 2024
411cd4c
Pretty
tom-andersen Feb 8, 2024
1d3cef2
Changelog
tom-andersen Feb 8, 2024
56872f4
Fix
tom-andersen Feb 8, 2024
a711b7d
Pretty
tom-andersen Feb 8, 2024
0d64d28
fix
tom-andersen Feb 8, 2024
c76b208
Merge branch 'master' into tomandersen/threadSafeObjectValue
tom-andersen Feb 9, 2024
c18bd0c
Doesn't need to be volatile
tom-andersen Feb 9, 2024
6841850
Re-write in hopes this will fix flaky test.
tom-andersen Feb 9, 2024
0bf0950
Merge remote-tracking branch 'origin/master' into tomandersen/threadS…
tom-andersen Feb 12, 2024
255a059
Merge root
tom-andersen Feb 12, 2024
4552533
Merge branch 'master' into tomandersen/threadSafeObjectValue
tom-andersen Feb 12, 2024
9b284f6
Revert
tom-andersen Feb 13, 2024
78d1c1e
Fix
tom-andersen Feb 13, 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
3 changes: 2 additions & 1 deletion firebase-firestore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Unreleased

* [fixed] Remove possible thread safety issues for ObjectValue. [#5699](//github.com/firebase/firebase-android-sdk/pull/5699)

# 24.10.2
* [changed] Internal test improvements.
Expand Down Expand Up @@ -890,3 +890,4 @@ updates.
or
[`FieldValue.serverTimestamp()`](/docs/reference/android/com/google/firebase/firestore/FieldValue.html#serverTimestamp())
values.

86 changes: 61 additions & 25 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ public final class ObjectValue implements Cloneable {
*/
private final Map<String, Object> overlayMap = new HashMap<>();

/**
* Dirty bit for when `overlayMap` is modified. The memoized `partialValue` only needs to be
* updated when dirty bit is true.
*/
private boolean dirty = false;

public static ObjectValue fromMap(Map<String, Value> value) {
return new ObjectValue(
Value.newBuilder().setMapValue(MapValue.newBuilder().putAllFields(value)).build());
Expand Down Expand Up @@ -103,32 +109,41 @@ private FieldMask extractFieldMask(MapValue value) {
}

@Nullable
private Value extractNestedValue(Value value, FieldPath fieldPath) {
private static Value extractNestedValue(Value value, FieldPath fieldPath) {
if (fieldPath.isEmpty()) {
return value;
} else {
for (int i = 0; i < fieldPath.length() - 1; ++i) {
value = value.getMapValue().getFieldsOrDefault(fieldPath.getSegment(i), null);
if (!Values.isMapValue(value)) {
value = extractSegmentValue(value, fieldPath.getSegment(i));
if (value == null) {
return null;
}
}
return value.getMapValue().getFieldsOrDefault(fieldPath.getLastSegment(), null);
return extractSegmentValue(value, fieldPath.getLastSegment());
}
}

private static Value extractSegmentValue(Value value, String segment) {
return Values.isMapValue(value) ? value.getMapValue().getFieldsOrDefault(segment, null) : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

/**
* Returns the Protobuf that backs this ObjectValue.
*
* <p>This method applies any outstanding modifications and memoizes the result. Further
* invocations are based on this memoized result.
*/
private Value buildProto() {
synchronized (overlayMap) {
MapValue mergedResult = applyOverlay(FieldPath.EMPTY_PATH, overlayMap);
if (mergedResult != null) {
partialValue = Value.newBuilder().setMapValue(mergedResult).build();
overlayMap.clear();
if (dirty) {
synchronized (overlayMap) {
if (dirty) {
MapValue mergedResult = applyOverlay(FieldPath.EMPTY_PATH, overlayMap, partialValue);
if (mergedResult != null) {
partialValue = Value.newBuilder().setMapValue(mergedResult).build();
}
overlayMap.clear();
dirty = false;
}
}
}
return partialValue;
Expand All @@ -142,7 +157,9 @@ private Value buildProto() {
*/
public void delete(FieldPath path) {
hardAssert(!path.isEmpty(), "Cannot delete field for empty path on ObjectValue");
setOverlay(path, null);
synchronized (overlayMap) {
setOverlay(path, null);
}
}

/**
Expand All @@ -153,16 +170,18 @@ public void delete(FieldPath path) {
*/
public void set(FieldPath path, Value value) {
hardAssert(!path.isEmpty(), "Cannot set field for empty path on ObjectValue");
setOverlay(path, value);
synchronized (overlayMap) {
setOverlay(path, value);
}
}

public void setAll(Map<FieldPath, Value> data) {
for (Map.Entry<FieldPath, Value> entry : data.entrySet()) {
FieldPath path = entry.getKey();
if (entry.getValue() == null) {
delete(path);
} else {
set(path, entry.getValue());
hardAssert(
!data.containsKey(FieldPath.EMPTY_PATH),
"Cannot setAll on ObjectValue because map contains empty path");
synchronized (overlayMap) {
for (Map.Entry<FieldPath, Value> entry : data.entrySet()) {
setOverlay(entry.getKey(), entry.getValue());
}
}
}
Expand Down Expand Up @@ -195,7 +214,19 @@ private void setOverlay(FieldPath path, @Nullable Value value) {
}
}

currentLevel.put(path.getLastSegment(), value);
String lastSegment = path.getLastSegment();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (value == null) { // Delete
// Get will return null if entry does not exist, so we must also do containsKey.
if (currentLevel.get(lastSegment) != null || !currentLevel.containsKey(lastSegment)) {
currentLevel.put(lastSegment, null);
dirty = true;
}
} else { // Set
if (!value.equals(currentLevel.get(lastSegment))) {
currentLevel.put(lastSegment, value);
dirty = true;
}
}
}

/**
Expand All @@ -206,18 +237,20 @@ private void setOverlay(FieldPath path, @Nullable Value value) {
* FieldValue.EMPTY_PATH} to represent the root.
* @param currentOverlays The overlays at the current nesting level in the same format as {@code
* overlayMap}.
* @param currentValue The existing value at the current nesting level.
* @return The merged data at `currentPath` or null if no modifications were applied.
*/
private @Nullable MapValue applyOverlay(
FieldPath currentPath, Map<String, Object> currentOverlays) {
private static @Nullable MapValue applyOverlay(
FieldPath currentPath, Map<String, Object> currentOverlays, Value currentValue) {
if (currentOverlays.isEmpty()) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this return currentValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
boolean modified = false;

@Nullable Value existingValue = extractNestedValue(partialValue, currentPath);
Copy link
Contributor Author

@tom-andersen tom-andersen Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

MapValue.Builder resultAtPath =
Values.isMapValue(existingValue)
Values.isMapValue(currentValue)
// If there is already data at the current path, base our modifications on top
// of the existing data.
? existingValue.getMapValue().toBuilder()
? currentValue.getMapValue().toBuilder()
: MapValue.newBuilder();

for (Map.Entry<String, Object> entry : currentOverlays.entrySet()) {
Expand All @@ -227,7 +260,10 @@ private void setOverlay(FieldPath path, @Nullable Value value) {
if (value instanceof Map) {
@Nullable
MapValue nested =
applyOverlay(currentPath.append(pathSegment), (Map<String, Object>) value);
applyOverlay(
currentPath.append(pathSegment),
(Map<String, Object>) value,
extractSegmentValue(currentValue, pathSegment));
if (nested != null) {
resultAtPath.putFields(pathSegment, Value.newBuilder().setMapValue(nested).build());
modified = true;
Expand Down