From 98f2c874c85a7e771f61fad67d6944bd549de1d9 Mon Sep 17 00:00:00 2001 From: Radek Felcman Date: Wed, 18 Dec 2024 16:12:23 +0100 Subject: [PATCH] ConcurrencyException "Max number of attempts to lock object" in WriteLockManager - bugfix - backport from master (#2329) * 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 (cherry picked from commit e4245e29fa3355029f10c42ea5546c73683dd504) --- .../internal/helper/WriteLockManager.java | 87 +++++++++---------- .../internal/sessions/AbstractSession.java | 5 +- ...latedClientSessionIdentityMapAccessor.java | 5 +- .../internal/sessions/ObjectChangeSet.java | 5 +- .../UnitOfWorkIdentityMapAccessor.java | 5 +- 5 files changed, 57 insertions(+), 50 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 be4a0964e91..e93e90b082b 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 @@ -131,9 +131,7 @@ public class WriteLockManager { /* the first element in this list will be the prevailing thread */ protected ExposedNodeLinkedList prevailingQueue; - 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 +178,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 +443,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 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 5a22647d3d7..32c7bab0cbf 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 @@ -2805,7 +2805,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(); @@ -2814,6 +2815,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 4dd2a256f45..4539a895894 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 @@ -263,7 +263,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 @@ -279,6 +280,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 a52aca6e766..6884215fdaf 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 @@ -534,7 +534,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); @@ -543,6 +544,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 52cdd963484..77149df3ec5 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 @@ -171,7 +171,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 @@ -184,6 +185,8 @@ protected Object getAndCloneCacheKeyFromParent(Object primaryKey, Object objectT } } catch (InterruptedException ex) { } + } finally { + cacheKey.getInstanceLock().unlock(); } // check for inheritance.