Skip to content

Commit cd067c2

Browse files
rfelcmanrfelcman
authored
NPE in QueryBasedValueHolder due to #2181 - bugfix (#2330)
 Signed-off-by: Radek Felcman <[email protected]>
1 parent 98f2c87 commit cd067c2

File tree

5 files changed

+50
-28
lines changed

5 files changed

+50
-28
lines changed

‎foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/indirection/IndirectList.java

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ public class IndirectList<E> extends Vector<E> implements CollectionChangeTracke
100100
*/
101101
private boolean useLazyInstantiation = true;
102102

103-
private final Lock instanceLock = new ReentrantLock();
103+
private Lock instanceLock = new ReentrantLock();
104104

105105
/**
106106
* PUBLIC:
@@ -355,16 +355,19 @@ before merging collections (again, "un-instantiated" collections are not merged)
355355
*/
356356
@Override
357357
public Object clone() {
358-
instanceLock.lock();
358+
//Keep origin pointer to lock in local variable as instance variable is updated inside
359+
Lock lock = instanceLock;
360+
lock.lock();
359361
try {
360362
IndirectList<E> result = (IndirectList<E>)super.clone();
361363
result.delegate = (Vector<E>)this.getDelegate().clone();
362364
result.valueHolder = new ValueHolder<>(result.delegate);
365+
result.instanceLock = new ReentrantLock();
363366
result.attributeName = null;
364367
result.changeListener = null;
365368
return result;
366369
} finally {
367-
instanceLock.unlock();
370+
lock.unlock();
368371
}
369372
}
370373

‎foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/indirection/IndirectMap.java

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ public class IndirectMap<K, V> extends Hashtable<K, V> implements CollectionChan
7676
/** Store load factor for lazy init. */
7777
protected float loadFactor = 0.75f;
7878

79-
private final Lock instanceLock = new ReentrantLock();
79+
private Lock instanceLock = new ReentrantLock();
8080

8181
/**
8282
* PUBLIC:
@@ -186,16 +186,19 @@ before merging collections (again, "un-instantiated" collections are not merged)
186186
*/
187187
@Override
188188
public Object clone() {
189-
instanceLock.lock();
189+
//Keep origin pointer to lock in local variable as instance variable is updated inside
190+
Lock lock = instanceLock;
191+
lock.lock();
190192
try {
191193
IndirectMap<K, V> result = (IndirectMap<K, V>)super.clone();
192194
result.delegate = (Hashtable<K, V>)this.getDelegate().clone();
193195
result.valueHolder = new ValueHolder<>(result.delegate);
196+
result.instanceLock = new ReentrantLock();
194197
result.attributeName = null;
195198
result.changeListener = null;
196199
return result;
197200
} finally {
198-
instanceLock.unlock();
201+
lock.unlock();
199202
}
200203
}
201204

‎foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/indirection/IndirectSet.java

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ public class IndirectSet<E> implements CollectionChangeTracker, Set<E>, Indirect
120120
*/
121121
private boolean useLazyInstantiation = false;
122122

123-
private final Lock instanceLock = new ReentrantLock();
123+
private Lock instanceLock = new ReentrantLock();
124124

125125
/**
126126
* Construct an empty IndirectSet.
@@ -297,12 +297,20 @@ before merging collections (again, "un-instantiated" collections are not merged)
297297
@Override
298298
public Object clone() {
299299
try {
300-
IndirectSet<E> result = (IndirectSet<E>)super.clone();
301-
result.delegate = this.cloneDelegate();
302-
result.valueHolder = new ValueHolder<>(result.delegate);
303-
result.attributeName = null;
304-
result.changeListener = null;
300+
//Keep origin pointer to lock in local variable as instance variable is updated inside
301+
Lock lock = instanceLock;
302+
lock.lock();
303+
try {
304+
IndirectSet<E> result = (IndirectSet<E>)super.clone();
305+
result.delegate = this.cloneDelegate();
306+
result.valueHolder = new ValueHolder<>(result.delegate);
307+
result.instanceLock = new ReentrantLock();
308+
result.attributeName = null;
309+
result.changeListener = null;
305310
return result;
311+
} finally {
312+
lock.unlock();
313+
}
306314
} catch (CloneNotSupportedException e) {
307315
throw new InternalError("clone not supported");
308316
}

‎foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/indirection/DatabaseValueHolder.java

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public abstract class DatabaseValueHolder<T> implements WeavedAttributeValueHold
5959
*/
6060
protected boolean isCoordinatedWithProperty = false;
6161

62-
private final Lock instanceLock = new ReentrantLock();
62+
private Lock instanceLock = new ReentrantLock();
6363

6464
/**
6565
* Default constructor.
@@ -70,7 +70,16 @@ protected DatabaseValueHolder() {
7070
@Override
7171
public Object clone() {
7272
try {
73-
return super.clone();
73+
//Keep origin pointer to lock in local variable as instance variable is updated inside
74+
Lock lock = instanceLock;
75+
lock.lock();
76+
try {
77+
DatabaseValueHolder<T> result = (DatabaseValueHolder<T>)super.clone();
78+
result.instanceLock = new ReentrantLock();
79+
return result;
80+
} finally {
81+
lock.unlock();
82+
}
7483
} catch (CloneNotSupportedException exception) {
7584
throw new InternalError();
7685
}
@@ -336,4 +345,8 @@ public String toString() {
336345
return "{" + Helper.getShortClassName(getClass()) + ": " + ToStringLocalization.buildMessage("not_instantiated", null) + "}";
337346
}
338347
}
348+
349+
Lock getInstanceLock() {
350+
return this.instanceLock;
351+
}
339352
}

‎foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/indirection/UnitOfWorkValueHolder.java

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,6 @@
2525
import org.eclipse.persistence.internal.sessions.UnitOfWorkImpl;
2626
import org.eclipse.persistence.logging.SessionLog;
2727

28-
import java.util.concurrent.locks.Lock;
29-
import java.util.concurrent.locks.ReentrantLock;
30-
3128
/**
3229
* A UnitOfWorkValueHolder is put in a clone object.
3330
* It wraps the value holder in the original object to delay
@@ -62,8 +59,6 @@ public abstract class UnitOfWorkValueHolder<T> extends DatabaseValueHolder<T> im
6259
protected String sourceAttributeName;
6360
protected ObjID wrappedValueHolderRemoteID;
6461

65-
private final Lock wrappedValueHolderLock = new ReentrantLock();
66-
6762
protected UnitOfWorkValueHolder() {
6863
super();
6964
}
@@ -151,10 +146,10 @@ protected T instantiateImpl() {
151146
Object value;
152147
// Bug 3835202 - Ensure access to valueholders is thread safe. Several of the methods
153148
// called below are not threadsafe alone.
154-
wrappedValueHolderLock.lock();
155-
try {
156-
if (this.wrappedValueHolder instanceof DatabaseValueHolder) {
157-
DatabaseValueHolder<T> wrapped = (DatabaseValueHolder<T>)this.wrappedValueHolder;
149+
if (this.wrappedValueHolder instanceof DatabaseValueHolder) {
150+
DatabaseValueHolder<T> wrapped = (DatabaseValueHolder<T>) this.wrappedValueHolder;
151+
wrapped.getInstanceLock().lock();
152+
try {
158153
UnitOfWorkImpl unitOfWork = getUnitOfWork();
159154
if (!wrapped.isEasilyInstantiated()) {
160155
if (wrapped.isPessimisticLockingValueHolder()) {
@@ -170,19 +165,19 @@ protected T instantiateImpl() {
170165
return wrapped.instantiateForUnitOfWorkValueHolder(this);
171166
}
172167
}
173-
if (!wrapped.isInstantiated()){
168+
if (!wrapped.isInstantiated()) {
174169
//if not instantiated then try and load the UOW versions to prevent the whole loading from the cache and cloning
175170
//process
176171
T result = wrapped.getValue((UnitOfWorkImpl) this.session);
177-
if (result != null){
172+
if (result != null) {
178173
return result;
179174
}
180175
}
176+
} finally {
177+
wrapped.getInstanceLock().unlock();
181178
}
182-
value = this.wrappedValueHolder.getValue();
183-
} finally {
184-
wrappedValueHolderLock.unlock();
185179
}
180+
value = this.wrappedValueHolder.getValue();
186181
return buildCloneFor(value);
187182
}
188183

0 commit comments

Comments
 (0)