Skip to content

Commit

Permalink
ConcurrencyException "Max number of attempts to lock object" in Write…
Browse files Browse the repository at this point in the history 
…LockManager - bugfix (#2323)

Fixes #2319 in `WriteLockManager` by `toWaitOn.getInstanceLockCondition().await(MAX_WAIT, TimeUnit.MILLISECONDS);` to wait for lock on object (CacheKey) to be released.
Before this fix there was `toWaitOnLockCondition.await(ConcurrencyUtil.SINGLETON.getAcquireWaitTime(), TimeUnit.MILLISECONDS);` which was `toWaitOnLockCondition.await(0, TimeUnit.MILLISECONDS);` in case of default settings -> Zero time ammount to wait.
Additionally there are some other migrations from `synchronized` to ReentrantLock usage for CacheKey related code.

Signed-off-by: Radek Felcman <[email protected]>
  • Loading branch information
@rfelcman
rfelcman authored Dec 16, 2024
1 parent 4ef3517 commit e4245e2
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 49 deletions.
86 changes: 41 additions & 45 deletions ...sistence.core/src/main/java/org/eclipse/persistence/internal/helper/WriteLockManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ public class WriteLockManager {

private final Lock toWaitOnLock = new ReentrantLock();
private final Lock instancePrevailingQueueLock = new ReentrantLock();
private final Condition toWaitOnLockCondition = toWaitOnLock.newCondition();

public WriteLockManager() {
this.prevailingQueue = new ExposedNodeLinkedList();
Expand Down Expand Up @@ -180,17 +179,16 @@ public Map acquireLocksForClone(Object objectForClone, ClassDescriptor descripto
// using the exact same approach we have been adding to the concurrency manager
ConcurrencyUtil.SINGLETON.determineIfReleaseDeferredLockAppearsToBeDeadLocked(toWaitOn, whileStartTimeMillis, lockManager, readLockManager, ALLOW_INTERRUPTED_EXCEPTION_TO_BE_FIRED_UP_TRUE);

toWaitOnLock.lock();
toWaitOn.getInstanceLock().lock();
try {
try {
if (toWaitOn.isAcquired()) {//last minute check to insure it is still locked.
toWaitOnLockCondition.await(ConcurrencyUtil.SINGLETON.getAcquireWaitTime(), TimeUnit.MILLISECONDS);// wait for lock on object to be released
}
} catch (InterruptedException ex) {
// Ignore exception thread should continue.
if (toWaitOn.isAcquired()) {//last minute check to insure it is still locked.
toWaitOn.getInstanceLockCondition().await(MAX_WAIT, TimeUnit.MILLISECONDS);// wait for lock on object to be released
}
} finally {
toWaitOnLock.unlock();
} catch (InterruptedException ex) {
// Ignore exception thread should continue.
}
finally {
toWaitOn.getInstanceLock().unlock();
}
Object waitObject = toWaitOn.getObject();
// Object may be null for loss of identity.
Expand Down Expand Up @@ -446,45 +444,43 @@ private void acquireRequiredLocksInternal(MergeManager mergeManager, UnitOfWorkC
// set the cache key on the merge manager for
// the object that could not be acquired
mergeManager.setWriteLockQueued(objectChangeSet.getId());
try {
if (activeCacheKey != null){
//wait on the lock of the object that we couldn't get.
activeCacheKey.getInstanceLock().lock();
try {
// verify that the cache key is still locked before we wait on it, as
//it may have been released since we tried to acquire it.
if (activeCacheKey.isAcquired() && (activeCacheKey.getActiveThread() != Thread.currentThread())) {
Thread thread = activeCacheKey.getActiveThread();
if (thread.isAlive()){
long time = System.currentTimeMillis();
activeCacheKey.getInstanceLockCondition().await(MAX_WAIT, TimeUnit.MILLISECONDS);
if (System.currentTimeMillis() - time >= MAX_WAIT){
Object[] params = new Object[]{MAX_WAIT /1000, descriptor.getJavaClassName(), activeCacheKey.getKey(), thread.getName()};
StringBuilder buffer = new StringBuilder(TraceLocalization.buildMessage("max_time_exceeded_for_acquirerequiredlocks_wait", params));
StackTraceElement[] trace = thread.getStackTrace();
for (StackTraceElement element : trace){
buffer.append("\t\tat");
buffer.append(element.toString());
buffer.append("\n");
}
session.log(SessionLog.SEVERE, SessionLog.CACHE, buffer.toString());
session.getIdentityMapAccessor().printIdentityMapLocks();
}
}else{
session.log(SessionLog.SEVERE, SessionLog.CACHE, "releasing_invalid_lock", new Object[] { thread.getName(),descriptor.getJavaClass(), objectChangeSet.getId()});
//thread that held lock is no longer alive. Something bad has happened like
while (activeCacheKey.isAcquired()){
// could have a depth greater than one.
activeCacheKey.release();
}
if (activeCacheKey != null){
//wait on the lock of the object that we couldn't get.
activeCacheKey.getInstanceLock().lock();
try {
// verify that the cache key is still locked before we wait on it, as
//it may have been released since we tried to acquire it.
if (activeCacheKey.isAcquired() && (activeCacheKey.getActiveThread() != Thread.currentThread())) {
Thread thread = activeCacheKey.getActiveThread();
if (thread.isAlive()){
long time = System.currentTimeMillis();
activeCacheKey.getInstanceLockCondition().await(MAX_WAIT, TimeUnit.MILLISECONDS);
if (System.currentTimeMillis() - time >= MAX_WAIT){
Object[] params = new Object[]{MAX_WAIT /1000, descriptor.getJavaClassName(), activeCacheKey.getKey(), thread.getName()};
StringBuilder buffer = new StringBuilder(TraceLocalization.buildMessage("max_time_exceeded_for_acquirerequiredlocks_wait", params));
StackTraceElement[] trace = thread.getStackTrace();
for (StackTraceElement element : trace){
buffer.append("\t\tat");
buffer.append(element.toString());
buffer.append("\n");
}
session.log(SessionLog.SEVERE, SessionLog.CACHE, buffer.toString());
session.getIdentityMapAccessor().printIdentityMapLocks();
}
}else{
session.log(SessionLog.SEVERE, SessionLog.CACHE, "releasing_invalid_lock", new Object[] { thread.getName(),descriptor.getJavaClass(), objectChangeSet.getId()});
//thread that held lock is no longer alive. Something bad has happened like
while (activeCacheKey.isAcquired()){
// could have a depth greater than one.
activeCacheKey.release();
}
} finally {
activeCacheKey.getInstanceLock().unlock();
}
}
} catch (InterruptedException exception) {
throw org.eclipse.persistence.exceptions.ConcurrencyException.waitWasInterrupted(exception.getMessage());
} catch (InterruptedException exception) {
throw org.eclipse.persistence.exceptions.ConcurrencyException.waitWasInterrupted(exception.getMessage());
} finally {
activeCacheKey.getInstanceLock().unlock();
}
}
// we want to record this information so that we have traceability over this sort of problems
addCacheKeyToMapWriteLockManagerToCacheKeysThatCouldNotBeAcquired(currentThread, activeCacheKey, timeWhenLocksToAcquireLoopStarted);
Expand Down
5 changes: 4 additions & 1 deletion ...istence.core/src/main/java/org/eclipse/persistence/internal/sessions/AbstractSession.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2811,7 +2811,8 @@ protected CacheKey getCacheKeyFromTargetSessionForMerge(Object implementation, O
cacheKey.acquireDeferredLock();
original = cacheKey.getObject();
if (original == null) {
synchronized (cacheKey) {
cacheKey.getInstanceLock().lock();
try {
if (cacheKey.isAcquired()) {
try {
cacheKey.wait();
Expand All @@ -2820,6 +2821,8 @@ protected CacheKey getCacheKeyFromTargetSessionForMerge(Object implementation, O
}
}
original = cacheKey.getObject();
} finally {
cacheKey.getInstanceLock().unlock();
}
}
cacheKey.releaseDeferredLock();
Expand Down
5 changes: 4 additions & 1 deletion ...a/org/eclipse/persistence/internal/sessions/IsolatedClientSessionIdentityMapAccessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,8 @@ protected Object getAndCloneCacheKeyFromParent(Object primaryKey, Object objectT
// in which GC could remove the object and we would end up with a null pointer
// as well we must inspect the cacheKey without locking on it.
if ((cacheKey != null) && (shouldReturnInvalidatedObjects || !descriptor.getCacheInvalidationPolicy().isInvalidated(cacheKey))) {
synchronized (cacheKey) {
cacheKey.getInstanceLock().lock();
try {
//if the object in the cachekey is null but the key is acquired then
//someone must be rebuilding it or creating a new one. Sleep until
// it's finished. A plain wait here would be more efficient but we may not
Expand All @@ -285,6 +286,8 @@ protected Object getAndCloneCacheKeyFromParent(Object primaryKey, Object objectT
if (objectFromCache == null) {
return null;
}
} finally {
cacheKey.getInstanceLock().unlock();
}
} else {
return null;
Expand Down
5 changes: 4 additions & 1 deletion ...istence.core/src/main/java/org/eclipse/persistence/internal/sessions/ObjectChangeSet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,8 @@ protected Object getObjectForMerge(MergeManager mergeManager, AbstractSession se
session.getParent().log(SessionLog.SEVERE, SessionLog.CACHE, "entity_not_available_during_merge", new Object[]{descriptor.getJavaClassName(), cacheKey.getKey(), Thread.currentThread().getName(), cacheKey.getActiveThread()});
break;
}
synchronized (cacheKey) {
cacheKey.getInstanceLock().lock();
try {
if (cacheKey.isAcquired()) {
try {
cacheKey.wait(10);
Expand All @@ -545,6 +546,8 @@ protected Object getObjectForMerge(MergeManager mergeManager, AbstractSession se
}
}
domainObject = cacheKey.getObject();
} finally {
cacheKey.getInstanceLock().unlock();
}
}
cacheKey.releaseDeferredLock();
Expand Down
5 changes: 4 additions & 1 deletion ...rc/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkIdentityMapAccessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,8 @@ protected Object getAndCloneCacheKeyFromParent(Object primaryKey, Object objectT
// in which GC could remove the object and we would end up with a null pointer
// as well we must inspect the cacheKey without locking on it.
if ((cacheKey != null) && (shouldReturnInvalidatedObjects || !descriptor.getCacheInvalidationPolicy().isInvalidated(cacheKey))) {
synchronized (cacheKey) {
cacheKey.getInstanceLock().lock();
try {
//if the object in the cachekey is null but the key is acquired then
//someone must be rebuilding it or creating a new one. Sleep until
// it's finished. A plain wait here would be more efficient but we may not
Expand All @@ -186,6 +187,8 @@ protected Object getAndCloneCacheKeyFromParent(Object primaryKey, Object objectT
}
} catch (InterruptedException ex) {
}
} finally {
cacheKey.getInstanceLock().unlock();
}

// check for inheritance.
Expand Down

0 comments on commit e4245e2

Please sign in to comment.