FISH-6291 Fix Data Synchronization Issue in a @Clustered @Singleton

appserver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/AbstractSingletonContainer.java

@@ -37,11 +37,12 @@
  * only if the new code is made subject to such option by the copyright
  * holder.
  */
-// Portions Copyright [2016-2021] [Payara Foundation and/or its affiliates]
+// Portions Copyright [2016-2023] [Payara Foundation and/or its affiliates]
 
 package com.sun.ejb.containers;
 
 import com.hazelcast.cp.IAtomicLong;
+import com.hazelcast.cp.lock.FencedLock;
 import com.hazelcast.map.IMap;
 import com.sun.ejb.ComponentContext;
 import com.sun.ejb.Container;
@@ -59,6 +60,7 @@
 import com.sun.enterprise.util.Utility;
 
 import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
 import java.rmi.RemoteException;
 import java.util.HashMap;
 import java.util.Map;
@@ -124,6 +126,7 @@ public abstract class AbstractSingletonContainer extends BaseContainer {
     protected final ClusteredSingletonLookup clusteredLookup =
         new ClusteredSingletonLookupImpl(ejbDescriptor, componentId);
 
+    protected FencedLock clusteredSingletonLock;
 
     /**
      * This constructor is called from the JarManager when a Jar is deployed.
@@ -182,13 +185,53 @@ protected EjbInvocation createEjbInvocation() {
 
     @Override
     protected ComponentContext _getContext(EjbInvocation invocation) throws EJBException {
+        // initialize, serialize the Singleton and set to the session
         checkInit();
         if (clusteredLookup.isClusteredEnabled()) {
             AbstractSessionContextImpl sessionContext = (AbstractSessionContextImpl) singletonCtx;
             try {
                 invocationManager.preInvoke(invocation);
                 invocation.context = sessionContext;
-                sessionContext.setEJB(clusteredLookup.getClusteredSingletonMap().get(clusteredLookup.getClusteredSessionKey()));
+
+                if (_logger.isLoggable(Level.FINE)) {
+                    // Log all operations on the lock - note this Should™ also log the call to unlock in releaseContext
+                    clusteredSingletonLock = (FencedLock) Proxy.newProxyInstance(loader, new Class<?>[]{FencedLock.class},
+                            (proxy, method, args) -> {
+                                FencedLock fencedLock = clusteredLookup.getDistributedLock();
+                                _logger.log(
+                                        Level.FINE,
+                                        "DistributedLock, about to call {0}, Locked: {1}, Locked by Us: {2}, thread ID {3}",
+                                        new Object[] {
+                                                method.getName(),
+                                                fencedLock.isLocked(),
+                                                fencedLock.isLockedByCurrentThread(),
+                                                Thread.currentThread().getId()});
+                                Object rv = method.invoke(fencedLock, args);
+                                _logger.log(
+                                        Level.FINE,
+                                        "DistributedLock, after call to {0}, Locked: {1}, Locked by Us: {2}, thread ID {3}",
+                                        new Object[] {
+                                                method.getName(),
+                                                fencedLock.isLocked(),
+                                                fencedLock.isLockedByCurrentThread(),
+                                                Thread.currentThread().getId()});
+                                return rv;
+                    });
+                } else {
+                    clusteredSingletonLock = clusteredLookup.getDistributedLock();
+                }
+
+                /**
+                 * Look up the clustered singleton and set it in the session context. Note that if this is an instance
+                 * of {@link CMCSingletonContainer} the
+                 * {@link CMCSingletonContainer#getClusteredSingleton(EjbInvocation)} will (by default)
+                 * guard concurrent access via {@link java.util.concurrent.locks.Lock Locks}.
+                 *
+                 * This is done here so that when multiple concurrent threads are queued up to execute they will
+                 * lock around the read & write of the EJB itself, rather than the method call.
+                 */
+                sessionContext.setEJB(getClusteredSingleton(invocation));
+
                 if (isJCDIEnabled()) {
                     if (sessionContext.getJCDIInjectionContext() != null) {
                         sessionContext.getJCDIInjectionContext().cleanup(false);
@@ -208,12 +251,25 @@ protected ComponentContext _getContext(EjbInvocation invocation) throws EJBExcep
         return singletonCtx;
     }
 
+    /**
+     * Get the clustered singleton for this container.
+     *
+     * This method does not provide any concurrent access guards, but may be overridden to do so.
+     *
+     * @param invocation The {@link EjbInvocation} that prompted the lookup
+     * @return The clustered singleton object
+     */
+    protected Object getClusteredSingleton(EjbInvocation invocation) {
+        return clusteredLookup.getClusteredSingletonMap().get(clusteredLookup.getClusteredSessionKey());
+    }
+
     @Override
     protected void releaseContext(EjbInvocation inv) throws EJBException {
         if (clusteredLookup.isClusteredEnabled()) {
             try {
                 invocationManager.preInvoke(inv);
                 if (clusteredLookup.getClusteredSingletonMap().containsKey(clusteredLookup.getClusteredSessionKey())) {
+                    // serializes the Singleton into Hazelcast
                     clusteredLookup.getClusteredSingletonMap().set(clusteredLookup.getClusteredSessionKey(), inv.context.getEJB());
                 }
             }

appserver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/CMCSingletonContainer.java

@@ -37,21 +37,17 @@
  * only if the new code is made subject to such option by the copyright
  * holder.
  */
-// Portions Copyright [2016-2021] [Payara Foundation and/or its affiliates]
+// Portions Copyright [2016-2023] [Payara Foundation and/or its affiliates]
 
 package com.sun.ejb.containers;
 
-import com.hazelcast.cp.lock.FencedLock;
 import com.sun.ejb.ComponentContext;
 import com.sun.ejb.EjbInvocation;
 import com.sun.ejb.InvocationInfo;
 import com.sun.ejb.MethodLockInfo;
-import static com.sun.ejb.containers.BaseContainer._logger;
 import com.sun.enterprise.security.SecurityManager;
-import java.lang.reflect.Proxy;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
-import java.util.logging.Level;
 import jakarta.ejb.ConcurrentAccessException;
 import jakarta.ejb.ConcurrentAccessTimeoutException;
 import jakarta.ejb.IllegalLoopbackException;
@@ -84,6 +80,35 @@ public CMCSingletonContainer(EjbDescriptor desc, ClassLoader cl, SecurityManager
         defaultMethodLockInfo.setLockType(LockType.WRITE, clusteredLookup.isDistributedLockEnabled());
     }
 
+    @Override
+    protected ComponentContext _getContext(EjbInvocation invocation) {
+        super._getContext(invocation);
+
+        MethodLockInfo lockInfo = (invocation.invocationInfo.methodLockInfo == null)
+                ? defaultMethodLockInfo : invocation.invocationInfo.methodLockInfo;
+        // If the lock is distributed we will have locked it during lookupClusteredSingleton, called from super._getContext
+        if (!lockInfo.isDistributed()) {
+            lock(invocation);
+        }
+
+        //Now that we have the lock return the singletonCtx
+        return singletonCtx;
+    }
+
+    /**
+     * Get the clustered singleton for this container.
+     *
+     * This overrides the method in {@link AbstractSingletonContainer} to guard concurrent access via {@link Lock Locks}
+     *
+     * @param invocation The {@link EjbInvocation} that prompted the lookup
+     * @return The clustered singleton object
+     */
+    @Override
+    protected Object getClusteredSingleton(EjbInvocation invocation) {
+        lock(invocation);
+        return super.getClusteredSingleton(invocation);
+    }
+
     /*
      * Findbugs complains that the lock acquired in this method is not
      *  unlocked on all paths in this method.
@@ -115,54 +140,35 @@ public CMCSingletonContainer(EjbDescriptor desc, ClassLoader cl, SecurityManager
      * before every bean method.
      *
      */
-    @Override
-    protected ComponentContext _getContext(EjbInvocation inv) {
-        super._getContext(inv);
-
-        InvocationInfo invInfo = inv.invocationInfo;
+    private void lock(EjbInvocation invocation) {
+        InvocationInfo invInfo = invocation.invocationInfo;
 
         MethodLockInfo lockInfo = (invInfo.methodLockInfo == null)
                 ? defaultMethodLockInfo : invInfo.methodLockInfo;
         Lock theLock;
         if(lockInfo.isDistributed()) {
-            if (_logger.isLoggable(Level.FINE)) {
-                // log all lock operations
-                theLock = (Lock) Proxy.newProxyInstance(loader, new Class<?>[]{Lock.class},
-                        (proxy, method, args) -> {
-                            FencedLock fencedLock = clusteredLookup.getDistributedLock();
-                            _logger.log(Level.FINE, "DistributedLock, about to call {0}, Locked: {1}, Locked by Us: {2}, thread ID {3}",
-                                    new Object[]{method.getName(), fencedLock.isLocked(), fencedLock.isLockedByCurrentThread(), Thread.currentThread().getId()});
-                            Object rv = method.invoke(fencedLock, args);
-                            _logger.log(Level.FINE, "DistributedLock, after to call {0}, Locked: {1}, Locked by Us: {2}, thread ID {3}",
-                                    new Object[]{method.getName(), fencedLock.isLocked(), fencedLock.isLockedByCurrentThread(), Thread.currentThread().getId()});
-                            return rv;
-                        });
-            } else {
-                theLock = clusteredLookup.getDistributedLock();
-            }
-        }
-        else {
+            theLock = clusteredSingletonLock;
+        } else {
             theLock = lockInfo.isReadLockedMethod() ? readLock : writeLock;
         }
 
         if ( (rwLock.getReadHoldCount() > 0) &&
-             (!rwLock.isWriteLockedByCurrentThread()) ) {
+                (!rwLock.isWriteLockedByCurrentThread()) ) {
             if( lockInfo.isWriteLockedMethod() ) {
                 throw new IllegalLoopbackException("Illegal Reentrant Access : Attempt to make " +
                         "a loopback call on a Write Lock method '" + invInfo.targetMethod1 +
                         "' while a Read lock is already held");
             }
         }
 
-
         /*
          * Please see comment at the beginning of the method.
          * Even though the method doesn't unlock the (possibly) acquired
          * lock, the lock is guaranteed to be unlocked in releaseContext()
          * even if exceptions were thrown in _getContext()
          */
         if (!lockInfo.hasTimeout() ||
-            ( (lockInfo.hasTimeout() && (lockInfo.getTimeout() == BLOCK_INDEFINITELY) )) ) {
+                ( (lockInfo.hasTimeout() && (lockInfo.getTimeout() == BLOCK_INDEFINITELY) )) ) {
             theLock.lock();
         } else {
             try {
@@ -186,12 +192,8 @@ protected ComponentContext _getContext(EjbInvocation inv) {
             }
         }
 
-
         //Now that we have acquired the lock, remember it
-        inv.setCMCLock(theLock);
-
-        //Now that we have the lock return the singletonCtx
-        return singletonCtx;
+        invocation.setCMCLock(theLock);
     }
 
     /**

appserver/payara-appserver-modules/payara-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusterScopeContext.java

@@ -1,7 +1,7 @@
 /*
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
  *
- * Copyright (c) [2016-2021] Payara Foundation and/or its affiliates. All rights reserved.
+ * Copyright (c) [2016-2023] Payara Foundation and/or its affiliates. All rights reserved.
  *
  * The contents of this file are subject to the terms of either the GNU
  * General Public License Version 2 only ("GPL") or the Common Development
@@ -41,6 +41,8 @@
 
 import java.lang.annotation.Annotation;
 
+import com.hazelcast.cp.lock.FencedLock;
+import fish.payara.cluster.DistributedLockType;
 import jakarta.enterprise.context.ApplicationScoped;
 import jakarta.enterprise.context.spi.Context;
 import jakarta.enterprise.context.spi.Contextual;
@@ -51,6 +53,7 @@
 import com.sun.enterprise.deployment.Application;
 import com.sun.enterprise.deployment.util.DOLUtils;
 
+import jakarta.interceptor.InvocationContext;
 import org.glassfish.internal.deployment.Deployment;
 import org.glassfish.soteria.cdi.CdiUtils;
 
@@ -81,14 +84,28 @@ public Class<? extends Annotation> getScope() {
 
     @Override
     public <TT> TT get(Contextual<TT> contextual, CreationalContext<TT> creationalContext) {
-        TT beanInstance = get(contextual);
-        if (beanInstance == null) {
-            beanInstance = getFromApplicationScoped(contextual, Optional.of(creationalContext));
-            final Bean<TT> bean = (Bean<TT>) contextual;
-            if (clusteredLookup.getClusteredSingletonMap()
-                    .putIfAbsent(getBeanName(bean, getAnnotation(beanManager, bean)), beanInstance) != null) {
-                bean.destroy(beanInstance, creationalContext);
-                beanInstance = get(contextual);
+        final Bean<TT> bean = (Bean<TT>) contextual;
+        Clustered clusteredAnnotation = getAnnotation(beanManager, bean);
+        clusteredLookup.setClusteredSessionKeyIfNotSet(bean.getBeanClass(), clusteredAnnotation);
+        boolean locked = lock(clusteredAnnotation, clusteredLookup.getDistributedLock());
+        TT beanInstance = null;
+        try {
+            beanInstance = get(contextual);
+            if (beanInstance == null) {
+                beanInstance = getFromApplicationScoped(contextual, Optional.of(creationalContext));
+                if (clusteredLookup.getClusteredSingletonMap()
+                        .putIfAbsent(getBeanName(bean, clusteredAnnotation), beanInstance) != null) {
+                    bean.destroy(beanInstance, creationalContext);
+                    beanInstance = get(contextual);
+                }
+            }
+        } finally {
+            /**
+             * If we couldn't find a bean instance we won't have unlocked in {@link ClusterScopedInterceptor#refreshAndUnlock(InvocationContext)},
+             * so unlock here as a fallback
+             */
+            if (locked && beanInstance == null) {
+                unlock(clusteredAnnotation, clusteredLookup.getDistributedLock());
             }
         }
         return beanInstance;
@@ -100,10 +117,23 @@ public <TT> TT get(Contextual<TT> contextual) {
         final Bean<TT> bean = (Bean<TT>) contextual;
         Clustered clusteredAnnotation = getAnnotation(beanManager, bean);
         String beanName = getBeanName(bean, clusteredAnnotation);
-        TT beanInstance = (TT)clusteredLookup.getClusteredSingletonMap().get(beanName);
-        if (clusteredAnnotation.callPostConstructOnAttach() && beanInstance != null &&
-                getFromApplicationScoped(contextual, Optional.empty()) == null ) {
-            beanManager.getContext(ApplicationScoped.class).get(contextual, beanManager.createCreationalContext(contextual));
+        clusteredLookup.setClusteredSessionKeyIfNotSet(bean.getBeanClass(), clusteredAnnotation);
+        boolean locked = lock(clusteredAnnotation, clusteredLookup.getDistributedLock());
+        TT beanInstance = null;
+        try {
+            beanInstance = (TT) clusteredLookup.getClusteredSingletonMap().get(beanName);
+            if (clusteredAnnotation.callPostConstructOnAttach() && beanInstance != null &&
+                    getFromApplicationScoped(contextual, Optional.empty()) == null) {
+                beanManager.getContext(ApplicationScoped.class).get(contextual, beanManager.createCreationalContext(contextual));
+            }
+        } finally {
+            /**
+             * If we couldn't find a bean instance we won't have unlocked in {@link ClusterScopedInterceptor#refreshAndUnlock(InvocationContext)},
+             * so unlock here as a fallback
+             */
+            if (locked && beanInstance == null) {
+                unlock(clusteredAnnotation, clusteredLookup.getDistributedLock());
+            }
         }
         return beanInstance;
     }
@@ -155,4 +185,22 @@ static String firstNonNull(String... items) {
         }
         throw new IllegalArgumentException("All elements were null.");
     }
+
+    private static boolean lock(Clustered clusteredAnnotation, FencedLock lock) {
+        if (clusteredAnnotation.lock() == DistributedLockType.LOCK) {
+            if (!lock.isLockedByCurrentThread()) {
+                lock.lock();
+                return true;
+            }
+        }
+        return false;
+    }
+
+    protected static void unlock(Clustered clusteredAnnotation, FencedLock lock) {
+        if (clusteredAnnotation.lock() == DistributedLockType.LOCK) {
+            if (lock.isLockedByCurrentThread()) {
+                lock.unlock();
+            }
+        }
+    }
 }