Fixes a bug that made concurrent access of a large nested IonStruct u…

…nsafe when only its parent had been made read-only.

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;
     }

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;
     }

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.

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,30 +269,151 @@ 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<CompletableFuture<Void>> 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"));
                     }
                 }));
             }
             waiting.forEach(CompletableFuture::join);
         }
     }
 
+    @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<CompletableFuture<Void>> 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
      */