From e4245e29fa3355029f10c42ea5546c73683dd504 Mon Sep 17 00:00:00 2001 From: Radek Felcman Date: Mon, 16 Dec 2024 15:44:27 +0100 Subject: [PATCH] ConcurrencyException "Max number of attempts to lock object" in WriteLockManager - 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 --- .../internal/helper/WriteLockManager.java | 86 +++++++++---------- .../internal/sessions/AbstractSession.java | 5 +- ...latedClientSessionIdentityMapAccessor.java | 5 +- .../internal/sessions/ObjectChangeSet.java | 5 +- .../UnitOfWorkIdentityMapAccessor.java | 5 +- 5 files changed, 57 insertions(+), 49 deletions(-) diff --git a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/helper/WriteLockManager.java b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/helper/WriteLockManager.java index 05bc45a7900..0a2013db4e6 100644 --- a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/helper/WriteLockManager.java +++ b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/helper/WriteLockManager.java @@ -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(); @@ -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. @@ -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); diff --git a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/AbstractSession.java b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/AbstractSession.java index 1296727bd42..870b0f4590b 100644 --- a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/AbstractSession.java +++ b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/AbstractSession.java @@ -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(); @@ -2820,6 +2821,8 @@ protected CacheKey getCacheKeyFromTargetSessionForMerge(Object implementation, O } } original = cacheKey.getObject(); + } finally { + cacheKey.getInstanceLock().unlock(); } } cacheKey.releaseDeferredLock(); diff --git a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/IsolatedClientSessionIdentityMapAccessor.java b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/IsolatedClientSessionIdentityMapAccessor.java index 07939ff02bb..666338896e1 100644 --- a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/IsolatedClientSessionIdentityMapAccessor.java +++ b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/IsolatedClientSessionIdentityMapAccessor.java @@ -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 @@ -285,6 +286,8 @@ protected Object getAndCloneCacheKeyFromParent(Object primaryKey, Object objectT if (objectFromCache == null) { return null; } + } finally { + cacheKey.getInstanceLock().unlock(); } } else { return null; diff --git a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/ObjectChangeSet.java b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/ObjectChangeSet.java index bc910625502..c0ae687f700 100644 --- a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/ObjectChangeSet.java +++ b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/ObjectChangeSet.java @@ -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); @@ -545,6 +546,8 @@ protected Object getObjectForMerge(MergeManager mergeManager, AbstractSession se } } domainObject = cacheKey.getObject(); + } finally { + cacheKey.getInstanceLock().unlock(); } } cacheKey.releaseDeferredLock(); diff --git a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkIdentityMapAccessor.java b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkIdentityMapAccessor.java index 62ca6dd2155..caf9c3a591a 100644 --- a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkIdentityMapAccessor.java +++ b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkIdentityMapAccessor.java @@ -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 @@ -186,6 +187,8 @@ protected Object getAndCloneCacheKeyFromParent(Object primaryKey, Object objectT } } catch (InterruptedException ex) { } + } finally { + cacheKey.getInstanceLock().unlock(); } // check for inheritance.