From 38c10f7e6bde49d68eae2c67bdf1b01a39f2453a Mon Sep 17 00:00:00 2001 From: Tyler Gregg Date: Wed, 14 Feb 2024 15:32:49 -0800 Subject: [PATCH] Fixes a bug that made concurrent access of a large nested IonStruct unsafe when only its parent had been made read-only. --- .../ion/impl/lite/IonContainerLite.java | 9 +- .../amazon/ion/impl/lite/IonStructLite.java | 5 +- .../amazon/ion/impl/lite/IonValueLite.java | 1 + src/test/java/com/amazon/ion/CloneTest.java | 136 +++++++++++++++++- 4 files changed, 141 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/lite/IonContainerLite.java b/src/main/java/com/amazon/ion/impl/lite/IonContainerLite.java index 934a1d58b6..f51376d9b6 100644 --- a/src/main/java/com/amazon/ion/impl/lite/IonContainerLite.java +++ b/src/main/java/com/amazon/ion/impl/lite/IonContainerLite.java @@ -743,7 +743,7 @@ final protected int nextSize(int current_size, boolean call_transition) } /** - * This is overriden in {@link IonStructLite} to add the {@link HashMap} of + * This is overridden in {@link IonStructLite} to add the {@link HashMap} of * field names when the struct becomes moderately large. * * @param size @@ -753,6 +753,13 @@ void transitionToLargeSize(int size) return; } + /** + * Force any lazy state to be materialized (if applicable). + */ + void forceMaterializationOfLazyState() { + + } + public final int get_child_count() { return _child_count; } diff --git a/src/main/java/com/amazon/ion/impl/lite/IonStructLite.java b/src/main/java/com/amazon/ion/impl/lite/IonStructLite.java index 23344a7497..2bd4f1715a 100644 --- a/src/main/java/com/amazon/ion/impl/lite/IonStructLite.java +++ b/src/main/java/com/amazon/ion/impl/lite/IonStructLite.java @@ -100,10 +100,8 @@ private void build_field_map() } @Override - public void makeReadOnly() { - // Eagerly initialize the fields map to prevent potential data races https://github.com/amazon-ion/ion-java/issues/629 + void forceMaterializationOfLazyState() { fieldMapIsActive(_child_count); - super.makeReadOnly(); } private void add_field(String fieldName, int newFieldIdx) @@ -383,6 +381,7 @@ public IonValue get(String fieldName) private boolean fieldMapIsActive(int proposedSize) { if (_field_map != null) return true; if (proposedSize <= STRUCT_INITIAL_SIZE) return false; + if (_isLocked()) return false; build_field_map(); return true; } diff --git a/src/main/java/com/amazon/ion/impl/lite/IonValueLite.java b/src/main/java/com/amazon/ion/impl/lite/IonValueLite.java index 7f28414fe5..e80b54833c 100644 --- a/src/main/java/com/amazon/ion/impl/lite/IonValueLite.java +++ b/src/main/java/com/amazon/ion/impl/lite/IonValueLite.java @@ -677,6 +677,7 @@ private boolean clearSymbolIDsIterative(boolean readOnlyMode) { holder.parent._isSymbolIdPresent(false); } if (readOnlyMode) { + holder.parent.forceMaterializationOfLazyState(); holder.parent._isLocked(true); } // The end of the container has been reached. Pop from the stack and update the parent's flag. diff --git a/src/test/java/com/amazon/ion/CloneTest.java b/src/test/java/com/amazon/ion/CloneTest.java index 9950de5a7e..e4c5f73ef5 100644 --- a/src/test/java/com/amazon/ion/CloneTest.java +++ b/src/test/java/com/amazon/ion/CloneTest.java @@ -18,11 +18,13 @@ import com.amazon.ion.impl._Private_IonSystem; import com.amazon.ion.junit.IonAssert; import com.amazon.ion.system.IonSystemBuilder; +import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; import java.util.ArrayList; import java.util.List; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicInteger; import static com.amazon.ion.impl._Private_Utils.newSymbolToken; import static org.hamcrest.CoreMatchers.containsString; @@ -34,6 +36,7 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; public class CloneTest { @@ -266,23 +269,31 @@ public void modifyAfterCloneDoesNotChangeOriginal() { assertNotEquals(original, clone); } - @Test - public void readOnlyIonStructMultithreadedTest() { + /** + * Verifies that a read-only struct without nested values can be accessed via multiple threads concurrently. + * @param cloneStruct true if the struct should be cloned before it is made read-only. + */ + public void testReadOnlyIonStructMultithreadedAccess(boolean cloneStruct) { // See: https://github.com/amazon-ion/ion-java/issues/629 String ionStr = "{a:1,b:2,c:3,d:4,e:5,f:6}"; IonStruct ionValue = (IonStruct)system().singleValue(ionStr); ionValue.makeReadOnly(); - for (int i=0; i<100; i++) { - IonStruct clone = ionValue.clone(); - clone.makeReadOnly(); + for (int i = 0; i < 100; i++) { + IonStruct struct; + if (cloneStruct) { + struct = ionValue.clone(); + } else { + struct = ionValue; + } + struct.makeReadOnly(); List> waiting = new ArrayList<>(); for (int j = 0; j < 4; j++) { waiting.add(CompletableFuture.runAsync(() -> { for (int k = 0; k <= 100; k++) { - assertNotNull(clone.get("a")); + assertNotNull(struct.get("a")); } })); } @@ -290,6 +301,119 @@ public void readOnlyIonStructMultithreadedTest() { } } + @RepeatedTest(100) + public void readOnlyIonStructMultithreadedAccessSucceeds() { + testReadOnlyIonStructMultithreadedAccess(false); + } + + @RepeatedTest(100) + public void readOnlyClonedIonStructMultithreadedAccessSucceeds() { + testReadOnlyIonStructMultithreadedAccess(true); + } + + /** + * Creates a new IonStruct with several children, including two large nested structs. The large children will + * take longer to clone and sequentially access, giving more time for race conditions to surface in multithreaded + * contexts if the code is insufficiently protected. + * @return a new IonStruct. + */ + private IonStruct createIonStructWithLargeChildStructs() { + IonSystem ionSystem = system(); + IonStruct attribute = ionSystem.newEmptyStruct(); + attribute.put("a", ionSystem.newSymbol("foo")); + attribute.put("b", ionSystem.newSymbol("foo")); + attribute.put("c", ionSystem.newSymbol("foo")); + IonStruct struct = ionSystem.newEmptyStruct(); + struct.put("a", ionSystem.newSymbol("foo")); + struct.put("b", ionSystem.newSymbol("foo")); + struct.put("c", ionSystem.newSymbol("foo")); + struct.put("number", ionSystem.newDecimal(1e-2)); + IonStruct child1 = ionSystem.newEmptyStruct(); + for (int i = 0; i < 8; i++) { + child1.put("field" + i, attribute.clone()); + } + struct.put("child1", child1); + IonStruct child2 = ionSystem.newEmptyStruct(); + for (int i = 0; i < 1000; i++) { + child2.put("beforeTarget" + i, attribute.clone()); + } + child2.put("target", attribute); + for (int i = 0; i < 400; i++) { + child2.put("afterTarget" + i, attribute.clone()); + } + struct.put("child2", child2); + return struct; + } + + /** + * Verifies that a read-only struct's large nested struct can be accessed via multiple threads concurrently. + * @param cloneStruct true if the struct should be cloned before it is made read-only. + * @param makeParentReadOnly if true, the top-level struct will be made only (cascading to all children); if false, + * only the child struct to be accessed concurrently will be made read-only. + */ + public void testReadOnlyIonStructMultithreadedNestedAccess(boolean cloneStruct, boolean makeParentReadOnly) { + IonStruct struct; + if (cloneStruct) { + struct = createIonStructWithLargeChildStructs().clone(); + } else { + struct = createIonStructWithLargeChildStructs(); + } + if (makeParentReadOnly) { + struct.makeReadOnly(); + } else { + struct.get("child2").makeReadOnly(); + } + + int numberOfTasks = 100; + int numberOfIterationsPerTask = 5; + AtomicInteger success = new AtomicInteger(); + AtomicInteger error = new AtomicInteger(); + List> tasks = new ArrayList<>(); + + for (int task = 0; task < numberOfTasks; task++) { + tasks.add(CompletableFuture.runAsync( + () -> { + for (int i = 0; i < numberOfIterationsPerTask; i++) { + IonStruct child2 = (IonStruct) struct.get("child2"); + String value = child2 == null || child2.get("target") == null + ? null + : ((IonStruct) child2.get("target")).get("a").toString(); + + if (value == null) { + error.getAndIncrement(); + break; + } + success.getAndIncrement(); + } + } + )); + } + tasks.forEach(CompletableFuture::join); + + assertTrue(success.get() > 0); + assertEquals(0, error.get()); + } + + @RepeatedTest(100) + public void readOnlyIonStructMultithreadedNestedAccessSucceeds() { + testReadOnlyIonStructMultithreadedNestedAccess(false, true); + } + + @RepeatedTest(100) + public void readOnlyClonedIonStructMultithreadedNestedAccessSucceeds() { + testReadOnlyIonStructMultithreadedNestedAccess(true, true); + } + + @RepeatedTest(100) + public void readOnlyNestedIonStructMultithreadedAccessSucceeds() { + testReadOnlyIonStructMultithreadedNestedAccess(false, false); + } + + @RepeatedTest(100) + public void readOnlyClonedNestedIonStructMultithreadedAccessSucceeds() { + testReadOnlyIonStructMultithreadedNestedAccess(true, false); + } + /** * @return the singleton IonSystem */