Revise event multicaster locking for non-synchronized retriever caching

Closes gh-25799

spring-context/src/main/java/org/springframework/context/event/AbstractApplicationEventMulticaster.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2020 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -63,9 +63,9 @@ import org.springframework.util.ObjectUtils;
 public abstract class AbstractApplicationEventMulticaster
 		implements ApplicationEventMulticaster, BeanClassLoaderAware, BeanFactoryAware {
 
-	private final ListenerRetriever defaultRetriever = new ListenerRetriever(false);
+	private final DefaultListenerRetriever defaultRetriever = new DefaultListenerRetriever();
 
-	final Map<ListenerCacheKey, ListenerRetriever> retrieverCache = new ConcurrentHashMap<>(64);
+	final Map<ListenerCacheKey, CachedListenerRetriever> retrieverCache = new ConcurrentHashMap<>(64);
 
 	@Nullable
 	private ClassLoader beanClassLoader;
@@ -73,8 +73,6 @@ public abstract class AbstractApplicationEventMulticaster
 	@Nullable
 	private ConfigurableBeanFactory beanFactory;
 
-	private Object retrievalMutex = this.defaultRetriever;
-
 
 	@Override
 	public void setBeanClassLoader(ClassLoader classLoader) {
@@ -90,7 +88,6 @@ public abstract class AbstractApplicationEventMulticaster
 		if (this.beanClassLoader == null) {
 			this.beanClassLoader = this.beanFactory.getBeanClassLoader();
 		}
-		this.retrievalMutex = this.beanFactory.getSingletonMutex();
 	}
 
 	private ConfigurableBeanFactory getBeanFactory() {
@@ -104,7 +101,7 @@ public abstract class AbstractApplicationEventMulticaster
 
 	@Override
 	public void addApplicationListener(ApplicationListener<?> listener) {
-		synchronized (this.retrievalMutex) {
+		synchronized (this.defaultRetriever) {
 			// Explicitly remove target for a proxy, if registered already,
 			// in order to avoid double invocations of the same listener.
 			Object singletonTarget = AopProxyUtils.getSingletonTarget(listener);
@@ -118,7 +115,7 @@ public abstract class AbstractApplicationEventMulticaster
 
 	@Override
 	public void addApplicationListenerBean(String listenerBeanName) {
-		synchronized (this.retrievalMutex) {
+		synchronized (this.defaultRetriever) {
 			this.defaultRetriever.applicationListenerBeans.add(listenerBeanName);
 			this.retrieverCache.clear();
 		}
@@ -126,7 +123,7 @@ public abstract class AbstractApplicationEventMulticaster
 
 	@Override
 	public void removeApplicationListener(ApplicationListener<?> listener) {
-		synchronized (this.retrievalMutex) {
+		synchronized (this.defaultRetriever) {
 			this.defaultRetriever.applicationListeners.remove(listener);
 			this.retrieverCache.clear();
 		}
@@ -134,7 +131,7 @@ public abstract class AbstractApplicationEventMulticaster
 
 	@Override
 	public void removeApplicationListenerBean(String listenerBeanName) {
-		synchronized (this.retrievalMutex) {
+		synchronized (this.defaultRetriever) {
 			this.defaultRetriever.applicationListenerBeans.remove(listenerBeanName);
 			this.retrieverCache.clear();
 		}
@@ -142,7 +139,7 @@ public abstract class AbstractApplicationEventMulticaster
 
 	@Override
 	public void removeAllListeners() {
-		synchronized (this.retrievalMutex) {
+		synchronized (this.defaultRetriever) {
 			this.defaultRetriever.applicationListeners.clear();
 			this.defaultRetriever.applicationListenerBeans.clear();
 			this.retrieverCache.clear();
@@ -156,7 +153,7 @@ public abstract class AbstractApplicationEventMulticaster
 	 * @see org.springframework.context.ApplicationListener
 	 */
 	protected Collection<ApplicationListener<?>> getApplicationListeners() {
-		synchronized (this.retrievalMutex) {
+		synchronized (this.defaultRetriever) {
 			return this.defaultRetriever.getApplicationListeners();
 		}
 	}
@@ -177,32 +174,34 @@ public abstract class AbstractApplicationEventMulticaster
 		Class<?> sourceType = (source != null ? source.getClass() : null);
 		ListenerCacheKey cacheKey = new ListenerCacheKey(eventType, sourceType);
 
-		// Quick check for existing entry on ConcurrentHashMap...
+		// Potential new retriever to populate
-		ListenerRetriever retriever = this.retrieverCache.get(cacheKey);
+		CachedListenerRetriever newRetriever = null;
-		if (retriever != null) {
-			return retriever.getApplicationListeners();
-		}
 
-		if (this.beanClassLoader == null ||
+		// Quick check for existing entry on ConcurrentHashMap
-				(ClassUtils.isCacheSafe(event.getClass(), this.beanClassLoader) &&
+		CachedListenerRetriever existingRetriever = this.retrieverCache.get(cacheKey);
-						(sourceType == null || ClassUtils.isCacheSafe(sourceType, this.beanClassLoader)))) {
+		if (existingRetriever == null) {
-			// Fully synchronized building and caching of a ListenerRetriever
+			// Caching a new ListenerRetriever if possible
-			synchronized (this.retrievalMutex) {
+			if (this.beanClassLoader == null ||
-				retriever = this.retrieverCache.get(cacheKey);
+					(ClassUtils.isCacheSafe(event.getClass(), this.beanClassLoader) &&
-				if (retriever != null) {
+							(sourceType == null || ClassUtils.isCacheSafe(sourceType, this.beanClassLoader)))) {
-					return retriever.getApplicationListeners();
+				newRetriever = new CachedListenerRetriever();
+				existingRetriever = this.retrieverCache.putIfAbsent(cacheKey, newRetriever);
+				if (existingRetriever != null) {
+					newRetriever = null;  // no need to populate it in retrieveApplicationListeners
 				}
-				retriever = new ListenerRetriever(true);
-				Collection<ApplicationListener<?>> listeners =
-						retrieveApplicationListeners(eventType, sourceType, retriever);
-				this.retrieverCache.put(cacheKey, retriever);
-				return listeners;
 			}
 		}
-		else {
+
-			// No ListenerRetriever caching -> no synchronization necessary
+		if (existingRetriever != null) {
-			return retrieveApplicationListeners(eventType, sourceType, null);
+			Collection<ApplicationListener<?>> result = existingRetriever.getApplicationListeners();
+			if (result != null) {
+				return result;
+			}
+			// If result is null, the existing retriever is not fully populated yet by another thread.
+			// Proceed like caching wasn't possible for this current local attempt.
 		}
+
+		return retrieveApplicationListeners(eventType, sourceType, newRetriever);
 	}
 
 	/**
@@ -213,12 +212,15 @@ public abstract class AbstractApplicationEventMulticaster
 	 * @return the pre-filtered list of application listeners for the given event and source type
 	 */
 	private Collection<ApplicationListener<?>> retrieveApplicationListeners(
-			ResolvableType eventType, @Nullable Class<?> sourceType, @Nullable ListenerRetriever retriever) {
+			ResolvableType eventType, @Nullable Class<?> sourceType, @Nullable CachedListenerRetriever retriever) {
 
 		List<ApplicationListener<?>> allListeners = new ArrayList<>();
+		Set<ApplicationListener<?>> filteredListeners = (retriever != null ? new LinkedHashSet<>() : null);
+		Set<String> filteredListenerBeans = (retriever != null ? new LinkedHashSet<>() : null);
+
 		Set<ApplicationListener<?>> listeners;
 		Set<String> listenerBeans;
-		synchronized (this.retrievalMutex) {
+		synchronized (this.defaultRetriever) {
 			listeners = new LinkedHashSet<>(this.defaultRetriever.applicationListeners);
 			listenerBeans = new LinkedHashSet<>(this.defaultRetriever.applicationListenerBeans);
 		}
@@ -228,7 +230,7 @@ public abstract class AbstractApplicationEventMulticaster
 		for (ApplicationListener<?> listener : listeners) {
 			if (supportsEvent(listener, eventType, sourceType)) {
 				if (retriever != null) {
-					retriever.applicationListeners.add(listener);
+					filteredListeners.add(listener);
 				}
 				allListeners.add(listener);
 			}
@@ -246,10 +248,10 @@ public abstract class AbstractApplicationEventMulticaster
 						if (!allListeners.contains(listener) && supportsEvent(listener, eventType, sourceType)) {
 							if (retriever != null) {
 								if (beanFactory.isSingleton(listenerBeanName)) {
-									retriever.applicationListeners.add(listener);
+									filteredListeners.add(listener);
 								}
 								else {
-									retriever.applicationListenerBeans.add(listenerBeanName);
+									filteredListenerBeans.add(listenerBeanName);
 								}
 							}
 							allListeners.add(listener);
@@ -261,7 +263,7 @@ public abstract class AbstractApplicationEventMulticaster
 						// BeanDefinition metadata (e.g. factory method generics) above.
 						Object listener = beanFactory.getSingleton(listenerBeanName);
 						if (retriever != null) {
-							retriever.applicationListeners.remove(listener);
+							filteredListeners.remove(listener);
 						}
 						allListeners.remove(listener);
 					}
@@ -274,9 +276,15 @@ public abstract class AbstractApplicationEventMulticaster
 		}
 
 		AnnotationAwareOrderComparator.sort(allListeners);
-		if (retriever != null && retriever.applicationListenerBeans.isEmpty()) {
+		if (retriever != null) {
-			retriever.applicationListeners.clear();
+			if (filteredListenerBeans.isEmpty()) {
-			retriever.applicationListeners.addAll(allListeners);
+				retriever.applicationListeners = new LinkedHashSet<>(allListeners);
+				retriever.applicationListenerBeans = filteredListenerBeans;
+			}
+			else {
+				retriever.applicationListeners = filteredListeners;
+				retriever.applicationListenerBeans = filteredListenerBeans;
+			}
 		}
 		return allListeners;
 	}
@@ -415,18 +423,55 @@ public abstract class AbstractApplicationEventMulticaster
 	 * allowing for efficient retrieval of pre-filtered listeners.
 	 * <p>An instance of this helper gets cached per event type and source type.
 	 */
-	private class ListenerRetriever {
+	private class CachedListenerRetriever {
+
+		@Nullable
+		public volatile Set<ApplicationListener<?>> applicationListeners;
+
+		@Nullable
+		public volatile Set<String> applicationListenerBeans;
+
+		@Nullable
+		public Collection<ApplicationListener<?>> getApplicationListeners() {
+			Set<ApplicationListener<?>> applicationListeners = this.applicationListeners;
+			Set<String> applicationListenerBeans = this.applicationListenerBeans;
+			if (applicationListeners == null || applicationListenerBeans == null) {
+				// Not fully populated yet
+				return null;
+			}
+
+			List<ApplicationListener<?>> allListeners = new ArrayList<>(
+					applicationListeners.size() + applicationListenerBeans.size());
+			allListeners.addAll(applicationListeners);
+			if (!applicationListenerBeans.isEmpty()) {
+				BeanFactory beanFactory = getBeanFactory();
+				for (String listenerBeanName : applicationListenerBeans) {
+					try {
+						allListeners.add(beanFactory.getBean(listenerBeanName, ApplicationListener.class));
+					}
+					catch (NoSuchBeanDefinitionException ex) {
+						// Singleton listener instance (without backing bean definition) disappeared -
+						// probably in the middle of the destruction phase
+					}
+				}
+			}
+			if (!applicationListenerBeans.isEmpty()) {
+				AnnotationAwareOrderComparator.sort(allListeners);
+			}
+			return allListeners;
+		}
+	}
+
+
+	/**
+	 * Helper class that encapsulates a general set of target listeners.
+	 */
+	private class DefaultListenerRetriever {
 
 		public final Set<ApplicationListener<?>> applicationListeners = new LinkedHashSet<>();
 
 		public final Set<String> applicationListenerBeans = new LinkedHashSet<>();
 
-		private final boolean preFiltered;
-
-		public ListenerRetriever(boolean preFiltered) {
-			this.preFiltered = preFiltered;
-		}
-
 		public Collection<ApplicationListener<?>> getApplicationListeners() {
 			List<ApplicationListener<?>> allListeners = new ArrayList<>(
 					this.applicationListeners.size() + this.applicationListenerBeans.size());
@@ -435,8 +480,9 @@ public abstract class AbstractApplicationEventMulticaster
 				BeanFactory beanFactory = getBeanFactory();
 				for (String listenerBeanName : this.applicationListenerBeans) {
 					try {
-						ApplicationListener<?> listener = beanFactory.getBean(listenerBeanName, ApplicationListener.class);
+						ApplicationListener<?> listener =
-						if (this.preFiltered || !allListeners.contains(listener)) {
+								beanFactory.getBean(listenerBeanName, ApplicationListener.class);
+						if (!allListeners.contains(listener)) {
 							allListeners.add(listener);
 						}
 					}
@@ -446,9 +492,7 @@ public abstract class AbstractApplicationEventMulticaster
 					}
 				}
 			}
-			if (!this.preFiltered || !this.applicationListenerBeans.isEmpty()) {
+			AnnotationAwareOrderComparator.sort(allListeners);
-				AnnotationAwareOrderComparator.sort(allListeners);
-			}
 			return allListeners;
 		}
 	}

spring-context/src/test/java/org/springframework/context/event/ApplicationContextEventTests.java

@@ -16,8 +16,8 @@
 
 package org.springframework.context.event;
 
+import java.util.ArrayList;
 import java.util.HashSet;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.Executor;
@@ -27,6 +27,7 @@ import org.junit.jupiter.api.Test;
 
 import org.springframework.aop.framework.ProxyFactory;
 import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.InitializingBean;
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.BeanPostProcessor;
 import org.springframework.beans.factory.config.RuntimeBeanReference;
@@ -35,6 +36,8 @@ import org.springframework.beans.testfixture.beans.TestBean;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
 import org.springframework.context.ApplicationEvent;
+import org.springframework.context.ApplicationEventPublisher;
+import org.springframework.context.ApplicationEventPublisherAware;
 import org.springframework.context.ApplicationListener;
 import org.springframework.context.PayloadApplicationEvent;
 import org.springframework.context.support.AbstractApplicationContext;
@@ -374,6 +377,19 @@ public class ApplicationContextEventTests extends AbstractApplicationEventListen
 		assertThat(MyNonSingletonListener.seenEvents.contains(event4)).isTrue();
 		MyNonSingletonListener.seenEvents.clear();
 
+		context.publishEvent(event1);
+		context.publishEvent(event2);
+		context.publishEvent(event3);
+		context.publishEvent(event4);
+		assertThat(MyNonSingletonListener.seenEvents.contains(event1)).isTrue();
+		assertThat(MyNonSingletonListener.seenEvents.contains(event2)).isTrue();
+		assertThat(MyNonSingletonListener.seenEvents.contains(event3)).isTrue();
+		assertThat(MyNonSingletonListener.seenEvents.contains(event4)).isTrue();
+		MyNonSingletonListener.seenEvents.clear();
+
+		AbstractApplicationEventMulticaster multicaster = context.getBean(AbstractApplicationEventMulticaster.class);
+		assertThat(multicaster.retrieverCache.size()).isEqualTo(3);
+
 		context.close();
 	}
 
@@ -516,6 +532,36 @@ public class ApplicationContextEventTests extends AbstractApplicationEventListen
 		context.close();
 	}
 
+	@Test
+	public void initMethodPublishesEvent() {
+		GenericApplicationContext context = new GenericApplicationContext();
+		context.registerBeanDefinition("listener", new RootBeanDefinition(BeanThatListens.class));
+		context.registerBeanDefinition("messageSource", new RootBeanDefinition(StaticMessageSource.class));
+		context.registerBeanDefinition("initMethod", new RootBeanDefinition(EventPublishingInitMethod.class));
+		context.refresh();
+
+		context.publishEvent(new MyEvent(this));
+		BeanThatListens listener = context.getBean(BeanThatListens.class);
+		assertThat(listener.getEventCount()).isEqualTo(3);
+
+		context.close();
+	}
+
+	@Test
+	public void initMethodPublishesAsyncEvent() {
+		GenericApplicationContext context = new GenericApplicationContext();
+		context.registerBeanDefinition("listener", new RootBeanDefinition(BeanThatListens.class));
+		context.registerBeanDefinition("messageSource", new RootBeanDefinition(StaticMessageSource.class));
+		context.registerBeanDefinition("initMethod", new RootBeanDefinition(AsyncEventPublishingInitMethod.class));
+		context.refresh();
+
+		context.publishEvent(new MyEvent(this));
+		BeanThatListens listener = context.getBean(BeanThatListens.class);
+		assertThat(listener.getEventCount()).isEqualTo(3);
+
+		context.close();
+	}
+
 
 	@SuppressWarnings("serial")
 	public static class MyEvent extends ApplicationEvent {
@@ -537,7 +583,7 @@ public class ApplicationContextEventTests extends AbstractApplicationEventListen
 
 	public static class MyOrderedListener1 implements ApplicationListener<ApplicationEvent>, Ordered {
 
-		public final List<ApplicationEvent> seenEvents = new LinkedList<>();
+		public final List<ApplicationEvent> seenEvents = new ArrayList<>();
 
 		@Override
 		public void onApplicationEvent(ApplicationEvent event) {
@@ -652,4 +698,38 @@ public class ApplicationContextEventTests extends AbstractApplicationEventListen
 		}
 	}
 
+
+	public static class EventPublishingInitMethod implements ApplicationEventPublisherAware, InitializingBean {
+
+		private ApplicationEventPublisher publisher;
+
+		@Override
+		public void setApplicationEventPublisher(ApplicationEventPublisher applicationEventPublisher) {
+			this.publisher = applicationEventPublisher;
+		}
+
+		@Override
+		public void afterPropertiesSet() throws Exception {
+			this.publisher.publishEvent(new MyEvent(this));
+		}
+	}
+
+
+	public static class AsyncEventPublishingInitMethod implements ApplicationEventPublisherAware, InitializingBean {
+
+		private ApplicationEventPublisher publisher;
+
+		@Override
+		public void setApplicationEventPublisher(ApplicationEventPublisher applicationEventPublisher) {
+			this.publisher = applicationEventPublisher;
+		}
+
+		@Override
+		public void afterPropertiesSet() throws Exception {
+			Thread thread = new Thread(() -> this.publisher.publishEvent(new MyEvent(this)));
+			thread.start();
+			thread.join();
+		}
+	}
+
 }