NPE in QueryBasedValueHolder due to #2181 - bugfix (#2330)

Signed-off-by: Radek Felcman <[email protected]>
eclipse-ee4j · Dec 18, 2024 · cd067c2 · cd067c2
1 parent 98f2c87
commit cd067c2
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 28 deletions.
diff --git a/...ipse.persistence.core/src/main/java/org/eclipse/persistence/indirection/IndirectList.java b/...ipse.persistence.core/src/main/java/org/eclipse/persistence/indirection/IndirectList.java
@@ -100,7 +100,7 @@ public class IndirectList<E> extends Vector<E> implements CollectionChangeTracke
      */
     private boolean useLazyInstantiation = true;
 
-    private final Lock instanceLock  = new ReentrantLock();
+    private Lock instanceLock  = new ReentrantLock();
 
     /**
      * PUBLIC:
@@ -355,16 +355,19 @@ before merging collections (again, "un-instantiated" collections are not merged)
     */
     @Override
     public Object clone() {
-        instanceLock.lock();
+        //Keep origin pointer to lock in local variable as instance variable is updated inside
+        Lock lock = instanceLock;
+        lock.lock();
         try {
             IndirectList<E> result = (IndirectList<E>)super.clone();
             result.delegate = (Vector<E>)this.getDelegate().clone();
             result.valueHolder = new ValueHolder<>(result.delegate);
+            result.instanceLock = new ReentrantLock();
             result.attributeName = null;
             result.changeListener = null;
             return result;
         } finally {
-            instanceLock.unlock();
+            lock.unlock();
         }
     }
 

diff --git a/...lipse.persistence.core/src/main/java/org/eclipse/persistence/indirection/IndirectMap.java b/...lipse.persistence.core/src/main/java/org/eclipse/persistence/indirection/IndirectMap.java
@@ -76,7 +76,7 @@ public class IndirectMap<K, V> extends Hashtable<K, V> implements CollectionChan
     /** Store load factor for lazy init. */
     protected float loadFactor = 0.75f;
 
-    private final Lock instanceLock  = new ReentrantLock();
+    private Lock instanceLock  = new ReentrantLock();
 
     /**
      * PUBLIC:
@@ -186,16 +186,19 @@ before merging collections (again, "un-instantiated" collections are not merged)
     */
     @Override
     public Object clone() {
-        instanceLock.lock();
+        //Keep origin pointer to lock in local variable as instance variable is updated inside
+        Lock lock = instanceLock;
+        lock.lock();
         try {
             IndirectMap<K, V> result = (IndirectMap<K, V>)super.clone();
             result.delegate = (Hashtable<K, V>)this.getDelegate().clone();
             result.valueHolder = new ValueHolder<>(result.delegate);
+            result.instanceLock = new ReentrantLock();
             result.attributeName = null;
             result.changeListener = null;
             return result;
         } finally {
-            instanceLock.unlock();
+            lock.unlock();
         }
     }
 

diff --git a/...lipse.persistence.core/src/main/java/org/eclipse/persistence/indirection/IndirectSet.java b/...lipse.persistence.core/src/main/java/org/eclipse/persistence/indirection/IndirectSet.java
@@ -120,7 +120,7 @@ public class IndirectSet<E> implements CollectionChangeTracker, Set<E>, Indirect
      */
     private boolean useLazyInstantiation = false;
 
-    private final Lock instanceLock  = new ReentrantLock();
+    private Lock instanceLock  = new ReentrantLock();
 
     /**
      * Construct an empty IndirectSet.
@@ -297,12 +297,20 @@ before merging collections (again, "un-instantiated" collections are not merged)
     @Override
     public Object clone() {
         try {
-            IndirectSet<E> result = (IndirectSet<E>)super.clone();
-            result.delegate = this.cloneDelegate();
-            result.valueHolder = new ValueHolder<>(result.delegate);
-            result.attributeName = null;
-            result.changeListener = null;
+            //Keep origin pointer to lock in local variable as instance variable is updated inside
+            Lock lock = instanceLock;
+            lock.lock();
+            try {
+                IndirectSet<E> result = (IndirectSet<E>)super.clone();
+                result.delegate = this.cloneDelegate();
+                result.valueHolder = new ValueHolder<>(result.delegate);
+                result.instanceLock = new ReentrantLock();
+                result.attributeName = null;
+                result.changeListener = null;
             return result;
+            } finally {
+                lock.unlock();
+            }
         } catch (CloneNotSupportedException e) {
             throw new InternalError("clone not supported");
         }

diff --git a/....core/src/main/java/org/eclipse/persistence/internal/indirection/DatabaseValueHolder.java b/....core/src/main/java/org/eclipse/persistence/internal/indirection/DatabaseValueHolder.java
@@ -59,7 +59,7 @@ public abstract class DatabaseValueHolder<T> implements WeavedAttributeValueHold
      */
     protected boolean isCoordinatedWithProperty = false;
 
-    private final Lock instanceLock  = new ReentrantLock();
+    private Lock instanceLock  = new ReentrantLock();
 
     /**
      * Default constructor.
@@ -70,7 +70,16 @@ protected DatabaseValueHolder() {
     @Override
     public Object clone() {
         try {
-            return super.clone();
+            //Keep origin pointer to lock in local variable as instance variable is updated inside
+            Lock lock = instanceLock;
+            lock.lock();
+            try {
+                DatabaseValueHolder<T> result = (DatabaseValueHolder<T>)super.clone();
+                result.instanceLock = new ReentrantLock();
+                return result;
+            } finally {
+                lock.unlock();
+            }
         } catch (CloneNotSupportedException exception) {
             throw new InternalError();
         }
@@ -336,4 +345,8 @@ public String toString() {
             return "{" + Helper.getShortClassName(getClass()) + ": " + ToStringLocalization.buildMessage("not_instantiated", null) + "}";
         }
     }
+
+    Lock getInstanceLock() {
+        return this.instanceLock;
+    }
 }
diff --git a/...ore/src/main/java/org/eclipse/persistence/internal/indirection/UnitOfWorkValueHolder.java b/...ore/src/main/java/org/eclipse/persistence/internal/indirection/UnitOfWorkValueHolder.java
@@ -25,9 +25,6 @@
 import org.eclipse.persistence.internal.sessions.UnitOfWorkImpl;
 import org.eclipse.persistence.logging.SessionLog;
 
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReentrantLock;
-
 /**
  * A UnitOfWorkValueHolder is put in a clone object.
  * It wraps the value holder in the original object to delay
@@ -62,8 +59,6 @@ public abstract class UnitOfWorkValueHolder<T> extends DatabaseValueHolder<T> im
     protected String sourceAttributeName;
     protected ObjID wrappedValueHolderRemoteID;
 
-    private final Lock wrappedValueHolderLock = new ReentrantLock();
-
     protected UnitOfWorkValueHolder() {
         super();
     }
@@ -151,10 +146,10 @@ protected T instantiateImpl() {
         Object value;
         // Bug 3835202 - Ensure access to valueholders is thread safe.  Several of the methods
         // called below are not threadsafe alone.
-        wrappedValueHolderLock.lock();
-        try {
-            if (this.wrappedValueHolder instanceof DatabaseValueHolder) {
-                DatabaseValueHolder<T> wrapped = (DatabaseValueHolder<T>)this.wrappedValueHolder;
+        if (this.wrappedValueHolder instanceof DatabaseValueHolder) {
+            DatabaseValueHolder<T> wrapped = (DatabaseValueHolder<T>) this.wrappedValueHolder;
+            wrapped.getInstanceLock().lock();
+            try {
                 UnitOfWorkImpl unitOfWork = getUnitOfWork();
                 if (!wrapped.isEasilyInstantiated()) {
                     if (wrapped.isPessimisticLockingValueHolder()) {
@@ -170,19 +165,19 @@ protected T instantiateImpl() {
                         return wrapped.instantiateForUnitOfWorkValueHolder(this);
                     }
                 }
-                if (!wrapped.isInstantiated()){
+                if (!wrapped.isInstantiated()) {
                     //if not instantiated then try and load the UOW versions to prevent the whole loading from the cache and cloning
                     //process
                     T result = wrapped.getValue((UnitOfWorkImpl) this.session);
-                    if (result != null){
+                    if (result != null) {
                         return result;
                     }
                 }
+            } finally {
+                wrapped.getInstanceLock().unlock();
             }
-            value = this.wrappedValueHolder.getValue();
-        } finally {
-            wrappedValueHolderLock.unlock();
         }
+        value = this.wrappedValueHolder.getValue();
         return buildCloneFor(value);
     }