FISH-796 Merge pull request payara#5012 from flowlogix/Fix-Clustered-…

…Singleton-Bugs FISH-796 Fix clustered singleton bugs and added test
aubi · Sep 27, 2021 · fc26970 · fc26970
1 parent bb8b3cc
commit fc26970
Show file tree

Hide file tree

Showing 7 changed files with 61 additions and 47 deletions.
diff --git a/.../java/com/sun/enterprise/container/common/impl/util/ClusteredSingletonLookupImplBase.java b/.../java/com/sun/enterprise/container/common/impl/util/ClusteredSingletonLookupImplBase.java
@@ -1,7 +1,7 @@
 /*
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
  *
- * Copyright (c) [2016-2019] Payara Foundation and/or its affiliates. All rights reserved.
+ * Copyright (c) [2016-2020] 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
@@ -57,14 +57,15 @@
  * @author lprimak
  */
 public abstract class ClusteredSingletonLookupImplBase implements ClusteredSingletonLookup {
-
     private final HazelcastCore hzCore = Globals.getDefaultHabitat().getService(HazelcastCore.class);
     private final String componentId;
     private final SingletonType singletonType;
     private final String keyPrefix;
     private final String mapKey;
     private final AtomicReference<String> sessionHzKey = new AtomicReference<>();
-    private final AtomicReference<String> lockKey = new AtomicReference<>();
+    private final AtomicReference<ILock> lock = new AtomicReference<>();
+    private final AtomicReference<IAtomicLong> count = new AtomicReference<>();
+
 
     public ClusteredSingletonLookupImplBase(String componentId, SingletonType singletonType) {
         this.componentId = componentId;
@@ -81,26 +82,13 @@ protected final String getMapKey() {
         return mapKey;
     }
 
-    protected final String getLockKey() {
-        return lockKey.updateAndGet(v -> v != null ? v : makeLockKey());
-    }
-
     public final String getSessionHzKey() {
         return sessionHzKey.updateAndGet(v -> v != null ? v : makeSessionHzKey());
     }
 
-    /**
-     * {@link #getSessionHzKey()} and {@link #getLockKey()} are dependent on {@link #getClusteredSessionKey()} so should
-     * its value change cache keys need to be invalidated using this method.
-     */
-    protected final void invalidateKeys() {
-        sessionHzKey.set(null);
-        lockKey.set(null);
-    }
-
     @Override
     public ILock getDistributedLock() {
-        return getHazelcastInstance().getLock(getLockKey());
+        return lock.updateAndGet(v -> v != null ? v : getHazelcastInstance().getLock(makeLockKey()));
     }
 
     @Override
@@ -109,8 +97,8 @@ public IMap<String, Object> getClusteredSingletonMap() {
     }
 
     @Override
-    public  IAtomicLong getClusteredUsageCount() {
-        return getHazelcastInstance().getAtomicLong(getSessionHzKey()+ "/count");
+    public IAtomicLong getClusteredUsageCount() {
+        return count.updateAndGet(v -> v != null ? v : getHazelcastInstance().getAtomicLong(makeCountKey()));
     }
 
     private HazelcastInstance getHazelcastInstance() {
@@ -130,6 +118,19 @@ public boolean isDistributedLockEnabled() {
         return isClusteredEnabled();
     }
 
+    @Override
+    public void destroy() {
+        getClusteredSingletonMap().delete(getClusteredSessionKey());
+
+        // CP locks and AtomicLong's can't be destroyed, as per https://github.com/hazelcast/hazelcast/issues/17498
+        // so we just release the references to them and reset to zero where we can
+        lock.set(null);
+        IAtomicLong oldCountValue = count.getAndSet(null);
+        if (oldCountValue != null) {
+            oldCountValue.set(0);
+        }
+    }
+
     @Override
     public HazelcastCore getHazelcastCore() {
         return hzCore;
@@ -147,6 +148,10 @@ private String makeLockKey() {
         return getSessionHzKey() + "/lock";
     }
 
+    private String makeCountKey() {
+        return getSessionHzKey() + "/count";
+    }
+
     private String makeSessionHzKey() {
         return getKeyPrefix() + componentId + "/" + getClusteredSessionKey();
     }

diff --git a/...ommon/src/main/java/com/sun/enterprise/container/common/spi/ClusteredSingletonLookup.java b/...ommon/src/main/java/com/sun/enterprise/container/common/spi/ClusteredSingletonLookup.java
@@ -1,7 +1,7 @@
 /*
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
  *
- * Copyright (c) [2016-2018] Payara Foundation and/or its affiliates. All rights reserved.
+ * Copyright (c) [2016-2020] 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
@@ -57,6 +57,10 @@ public interface ClusteredSingletonLookup {
     String getClusteredSessionKey();
     boolean isClusteredEnabled();
     IAtomicLong getClusteredUsageCount();
+    /**
+     * destroys usage count and distributed lock objects
+     */
+    void destroy();
     HazelcastCore getHazelcastCore();
 
     enum SingletonType {

diff --git a/...er/ejb/ejb-container/src/main/java/com/sun/ejb/containers/AbstractSingletonContainer.java b/...er/ejb/ejb-container/src/main/java/com/sun/ejb/containers/AbstractSingletonContainer.java
@@ -37,7 +37,7 @@
  * only if the new code is made subject to such option by the copyright
  * holder.
  */
-// Portions Copyright [2016-2019] [Payara Foundation and/or its affiliates]
+// Portions Copyright [2016-2020] [Payara Foundation and/or its affiliates]
 
 package com.sun.ejb.containers;
 
@@ -121,7 +121,7 @@ public abstract class AbstractSingletonContainer extends BaseContainer {
     private final InvocationInfo postConstructInvInfo;
     private final InvocationInfo preDestroyInvInfo;
 
-    protected final ClusteredSingletonLookup clusteredLookup = //
+    protected final ClusteredSingletonLookup clusteredLookup =
         new ClusteredSingletonLookupImpl(ejbDescriptor, componentId);
 
 
@@ -712,8 +712,7 @@ public void destroy(Object obj) {
                 EjbSessionDescriptor sessDesc = (EjbSessionDescriptor) ejbDescriptor;
                 IAtomicLong count = clusteredLookup.getClusteredUsageCount();
                 if (count.decrementAndGet() <= 0) {
-                    clusteredLookup.getClusteredSingletonMap().delete(clusteredLookup.getClusteredSessionKey());
-                    count.destroy();
+                    clusteredLookup.destroy();
                 } else if (sessDesc.dontCallPreDestroyOnDetach()) {
                     doPreDestroy = false;
                 }

diff --git a/...-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusterScopeContext.java b/...-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-2019] Payara Foundation and/or its affiliates. All rights reserved.
+ * Copyright (c) [2016-2020] 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
@@ -121,12 +121,12 @@ private<TT> TT getFromApplicationScoped(Contextual<TT> contextual, Optional<Crea
             return beanManager.getContext(ApplicationScoped.class).get(contextual);
         }
     }
-    
+
     /**
      * Get the most appropriate name for a bean. First checks the `@Clustered`
      * annotation key name property, then the bean EL name, then the bean class
      * name.
-     * 
+     *
      * @param bean       the bean to reference.
      * @param annotation the Clustered annotation to reference.
      * @throws IllegalArgumentException if no name can be found for the bean.
@@ -147,7 +147,7 @@ static <TT> Clustered getAnnotation(BeanManager beanManager, Class<?> clazz) {
         return CdiUtils.getAnnotation(beanManager, clazz, Clustered.class).get();
     }
 
-    private static String firstNonNull(String... items) {
+    static String firstNonNull(String... items) {
         for (String i : items) {
             if (i != null && !i.trim().isEmpty()) {
                 return i;

diff --git a/...o-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusterScopedInterceptor.java b/...o-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusterScopedInterceptor.java
@@ -1,7 +1,7 @@
 /*
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
  *
- * Copyright (c) [2016-2019] Payara Foundation and/or its affiliates. All rights reserved.
+ * Copyright (c) [2016-2020] 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
@@ -44,7 +44,6 @@
 import fish.payara.cluster.DistributedLockType;
 import static fish.payara.micro.cdi.extension.cluster.ClusterScopeContext.getAnnotation;
 import static fish.payara.micro.cdi.extension.cluster.ClusterScopeContext.getBeanName;
-import fish.payara.micro.cdi.extension.cluster.annotations.ClusterScoped;
 import fish.payara.micro.cdi.extension.cluster.annotations.ClusterScopedIntercepted;
 import java.io.IOException;
 import java.io.ObjectInputStream;
@@ -53,7 +52,6 @@
 import javax.annotation.PostConstruct;
 import javax.annotation.PreDestroy;
 import javax.annotation.Priority;
-import javax.enterprise.context.spi.Context;
 import javax.enterprise.inject.spi.Bean;
 import javax.enterprise.inject.spi.BeanManager;
 import javax.enterprise.inject.spi.CDI;
@@ -88,15 +86,16 @@ public Object lockAndRefresh(InvocationContext invocationContext) throws Excepti
             return invocationContext.proceed();
         }
         finally {
-            refresh(beanClass);
+            refresh(beanClass, invocationContext.getTarget());
             unlock(beanClass, clusteredAnnotation);
         }
     }
 
     @PostConstruct
     Object postConstruct(InvocationContext invocationContext) throws Exception {
         Class<?> beanClass = invocationContext.getTarget().getClass().getSuperclass();
-        clusteredLookup.setClusteredSessionKey(beanClass);
+        Clustered clusteredAnnotation = getAnnotation(beanManager, beanClass);
+        clusteredLookup.setClusteredSessionKeyIfNotSet(beanClass, clusteredAnnotation);
         clusteredLookup.getClusteredUsageCount().incrementAndGet();
         return invocationContext.proceed();
     }
@@ -105,11 +104,10 @@ Object postConstruct(InvocationContext invocationContext) throws Exception {
     Object preDestroy(InvocationContext invocationContext) throws Exception {
         Class<?> beanClass = invocationContext.getTarget().getClass().getSuperclass();
         Clustered clusteredAnnotation = getAnnotation(beanManager, beanClass);
-        clusteredLookup.setClusteredSessionKey(beanClass);
+        clusteredLookup.setClusteredSessionKeyIfNotSet(beanClass, clusteredAnnotation);
         IAtomicLong count = clusteredLookup.getClusteredUsageCount();
         if (count.decrementAndGet() <= 0) {
-            clusteredLookup.getClusteredSingletonMap().delete(clusteredLookup.getClusteredSessionKey());
-            count.destroy();
+            clusteredLookup.destroy();
         } else if (!clusteredAnnotation.callPreDestoyOnDetach()) {
             return null;
         }
@@ -119,27 +117,26 @@ Object preDestroy(InvocationContext invocationContext) throws Exception {
 
     private void lock(Class<?> beanClass, Clustered clusteredAnnotation) {
         if (clusteredAnnotation.lock() == DistributedLockType.LOCK) {
-            clusteredLookup.setClusteredSessionKey(beanClass);
+            clusteredLookup.setClusteredSessionKeyIfNotSet(beanClass, clusteredAnnotation);
             clusteredLookup.getDistributedLock().lock();
         }
     }
 
     private void unlock(Class<?> beanClass, Clustered clusteredAnnotation) {
         if (clusteredAnnotation.lock() == DistributedLockType.LOCK) {
-            clusteredLookup.setClusteredSessionKey(beanClass);
+            clusteredLookup.setClusteredSessionKeyIfNotSet(beanClass, clusteredAnnotation);
             clusteredLookup.getDistributedLock().unlock();
         }
     }
 
-    private void refresh(Class<?> beanClass) {
+    private void refresh(Class<?> beanClass, Object instance) {
         Set<Bean<?>> managedBeans = beanManager.getBeans(beanClass);
         if (managedBeans.size() > 1) {
             throw new IllegalArgumentException("Multiple beans found for " + beanClass);
         }
         Bean<?> bean = managedBeans.iterator().next();
         String beanName = getBeanName(bean, getAnnotation(beanManager, bean));
-        Context ctx = beanManager.getContext(ClusterScoped.class);
-        clusteredLookup.getClusteredSingletonMap().put(beanName, ctx.get(bean));
+        clusteredLookup.getClusteredSingletonMap().set(beanName, instance);
     }
 
     private void init() {

diff --git a/...i/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusteredSingletonLookupImpl.java b/...i/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusteredSingletonLookupImpl.java
@@ -1,7 +1,7 @@
 /*
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
  *
- * Copyright (c) [2016-2018] Payara Foundation and/or its affiliates. All rights reserved.
+ * Copyright (c) [2016-2020] 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,9 +41,11 @@
 
 import com.sun.enterprise.container.common.impl.util.ClusteredSingletonLookupImplBase;
 import static com.sun.enterprise.container.common.spi.ClusteredSingletonLookup.SingletonType.CDI;
+import fish.payara.cluster.Clustered;
 import static fish.payara.micro.cdi.extension.cluster.ClusterScopeContext.getAnnotation;
 import static fish.payara.micro.cdi.extension.cluster.ClusterScopeContext.getBeanName;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
 import javax.enterprise.inject.spi.Bean;
 import javax.enterprise.inject.spi.BeanManager;
 
@@ -53,9 +55,8 @@
  * @author lprimak
  */
 public class ClusteredSingletonLookupImpl extends ClusteredSingletonLookupImplBase {
-
     private final BeanManager beanManager;
-    private final ThreadLocal<String> sessionKey = new ThreadLocal<>();
+    private final AtomicReference<String> sessionKey = new AtomicReference<>();
 
     public ClusteredSingletonLookupImpl(BeanManager beanManager, String componentId) {
         super(componentId, CDI);
@@ -67,15 +68,19 @@ public String getClusteredSessionKey() {
         return sessionKey.get();
     }
 
-    void setClusteredSessionKey(Class<?> beanClass) {
+    void setClusteredSessionKeyIfNotSet(Class<?> beanClass, Clustered clusteredAnnotation) {
+        sessionKey.updateAndGet(v -> v != null ? v : makeSessionKey(beanClass, clusteredAnnotation));
+    }
+
+    private String makeSessionKey(Class<?> beanClass, Clustered clusteredAnnotation) {
         Set<Bean<?>> managedBeans = beanManager.getBeans(beanClass);
         if (managedBeans.size() > 1) {
             throw new IllegalArgumentException("Multiple beans found for " + beanClass);
         }
         if (managedBeans.size() == 1) {
             Bean<?> bean = managedBeans.iterator().next();
-            sessionKey.set(getBeanName(bean, getAnnotation(beanManager, bean)));
-            invalidateKeys();
+            return getBeanName(bean, getAnnotation(beanManager, bean));
         }
+        return ClusterScopeContext.firstNonNull(clusteredAnnotation.keyName(), beanClass.getName());
     }
 }
diff --git a/...c/main/java/fish/payara/samples/clustered/singleton/ClusteredSingletonInterceptedEJB.java b/...c/main/java/fish/payara/samples/clustered/singleton/ClusteredSingletonInterceptedEJB.java
@@ -1,7 +1,11 @@
 /*
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
  *
+<<<<<<< HEAD
  * Copyright (c) [2016-2021] Payara Foundation and/or its affiliates. All rights reserved.
+=======
+ * Copyright (c) [2016-2020] Payara Foundation and/or its affiliates. All rights reserved.
+>>>>>>> a62d616b29 (Merge pull request #5012 from flowlogix/Fix-Clustered-Singleton-Bugs)
  *
  * 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