From fc26970edf503f5d501bd904558444dfd139efec Mon Sep 17 00:00:00 2001 From: Matt Gill Date: Tue, 8 Dec 2020 14:35:54 +0000 Subject: [PATCH] FISH-796 Merge pull request #5012 from flowlogix/Fix-Clustered-Singleton-Bugs FISH-796 Fix clustered singleton bugs and added test --- .../ClusteredSingletonLookupImplBase.java | 43 +++++++++++-------- .../common/spi/ClusteredSingletonLookup.java | 6 ++- .../AbstractSingletonContainer.java | 7 ++- .../cluster/ClusterScopeContext.java | 8 ++-- .../cluster/ClusterScopedInterceptor.java | 23 +++++----- .../cluster/ClusteredSingletonLookupImpl.java | 17 +++++--- .../ClusteredSingletonInterceptedEJB.java | 4 ++ 7 files changed, 61 insertions(+), 47 deletions(-) diff --git a/appserver/common/container-common/src/main/java/com/sun/enterprise/container/common/impl/util/ClusteredSingletonLookupImplBase.java b/appserver/common/container-common/src/main/java/com/sun/enterprise/container/common/impl/util/ClusteredSingletonLookupImplBase.java index b992b28b64e..e4828842ff6 100644 --- a/appserver/common/container-common/src/main/java/com/sun/enterprise/container/common/impl/util/ClusteredSingletonLookupImplBase.java +++ b/appserver/common/container-common/src/main/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 sessionHzKey = new AtomicReference<>(); - private final AtomicReference lockKey = new AtomicReference<>(); + private final AtomicReference lock = new AtomicReference<>(); + private final AtomicReference 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 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/appserver/common/container-common/src/main/java/com/sun/enterprise/container/common/spi/ClusteredSingletonLookup.java b/appserver/common/container-common/src/main/java/com/sun/enterprise/container/common/spi/ClusteredSingletonLookup.java index b74653d9d92..caa8fc667d0 100644 --- a/appserver/common/container-common/src/main/java/com/sun/enterprise/container/common/spi/ClusteredSingletonLookup.java +++ b/appserver/common/container-common/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/appserver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/AbstractSingletonContainer.java b/appserver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/AbstractSingletonContainer.java index 1daa6b011b5..12dfe41829e 100644 --- a/appserver/ejb/ejb-container/src/main/java/com/sun/ejb/containers/AbstractSingletonContainer.java +++ b/appserver/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/appserver/payara-appserver-modules/payara-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusterScopeContext.java b/appserver/payara-appserver-modules/payara-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusterScopeContext.java index 873e5b672b3..e4c7322e09c 100644 --- a/appserver/payara-appserver-modules/payara-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusterScopeContext.java +++ b/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-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 getFromApplicationScoped(Contextual contextual, Optional 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/appserver/payara-appserver-modules/payara-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusterScopedInterceptor.java b/appserver/payara-appserver-modules/payara-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusterScopedInterceptor.java index f52ed943514..0676e8c94d7 100644 --- a/appserver/payara-appserver-modules/payara-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusterScopedInterceptor.java +++ b/appserver/payara-appserver-modules/payara-micro-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,7 +86,7 @@ public Object lockAndRefresh(InvocationContext invocationContext) throws Excepti return invocationContext.proceed(); } finally { - refresh(beanClass); + refresh(beanClass, invocationContext.getTarget()); unlock(beanClass, clusteredAnnotation); } } @@ -96,7 +94,8 @@ public Object lockAndRefresh(InvocationContext invocationContext) throws Excepti @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> 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/appserver/payara-appserver-modules/payara-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusteredSingletonLookupImpl.java b/appserver/payara-appserver-modules/payara-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusteredSingletonLookupImpl.java index 6a34071061b..8e2d261e8b6 100644 --- a/appserver/payara-appserver-modules/payara-micro-cdi/src/main/java/fish/payara/micro/cdi/extension/cluster/ClusteredSingletonLookupImpl.java +++ b/appserver/payara-appserver-modules/payara-micro-cdi/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 sessionKey = new ThreadLocal<>(); + private final AtomicReference 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> 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/appserver/tests/payara-samples/samples/clustered-singleton/clustered-singleton-test/src/main/java/fish/payara/samples/clustered/singleton/ClusteredSingletonInterceptedEJB.java b/appserver/tests/payara-samples/samples/clustered-singleton/clustered-singleton-test/src/main/java/fish/payara/samples/clustered/singleton/ClusteredSingletonInterceptedEJB.java index 6b4a3364711..2f7a9c65eed 100644 --- a/appserver/tests/payara-samples/samples/clustered-singleton/clustered-singleton-test/src/main/java/fish/payara/samples/clustered/singleton/ClusteredSingletonInterceptedEJB.java +++ b/appserver/tests/payara-samples/samples/clustered-singleton/clustered-singleton-test/src/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