Spring Using AOP and DI itself causes Spring ApplicationListener to be fired twice

Software versions

Spring Version 5.3.18 and earlier
JDK Version 1.8.0_202

Overview

When I use Spring ApplicationListener, in order to prevent transaction invalidation, my ApplicationListener implementation class writes the following code (of course, the code can be written differently to avoid this problem), which will cause my listener to trigger twice after the event is published. I think it's not normal, but not sure if it's a bug, so I want to ask everyone's opinion.

@Component
public class EventDemoListener implements ApplicationListener<EventDemo> {
    @Autowired
    DemoService1 demoService1;
    @Autowired
    DemoService2 demoService2;
    @Autowired
    EventDemoListener eventDemoListener;

    @Override
    public void onApplicationEvent(EventDemo event) {
        eventDemoListener.testTransaction();
        System.out.println("receiver " + event.getMessage());
    }

    @Transactional(rollbackFor = Exception.class)
    public void testTransaction() {
        demoService1.doService();
        demoService2.doService();
    }
}

Through this demo project, this problem can be reproduced. Please read the README.md document before running.
https://github.com/ZiFeng-Wu/spring-study

Analysis

After analysis, because here DI itself , When EventDemoListener is created, property filling will trigger DefaultSingletonBeanRegistry#getSingleton(String, boolean) in advance.
Then singletonFactory.getObject() executed in getSingleton() will cause the unproxyed EventDemoListener object to be put into AbstractAutoProxyCreator#earlyProxyReferences.
After the properties are filled, calling AbstractAutowireCapableBeanFactory#initializeBean(String, Object, RootBeanDefinition) and executing ApplicationListenerDetector#postProcessAfterInitialization(Object, String) will cause the unproxyed EventDemoListener object to be put into the AbstractApplicationEventMulticaster.DefaultListenerRetriever#applicationListeners container.
Then when the event is published, execute AbstractApplicationEventMulticaster.DefaultListenerRetriever#getApplicationListeners() and use ApplicationListener<?> listener =beanFactory.getBean(listenerBeanName, ApplicationListener.class) to obtain the listener is the proxied EventDemoListener object.
At this time, there are only unproxyed EventDemoListener object in the applicationListeners container, so the proxied EventDemoListener object will be added to the final returned allListeners collection, as shown in the figure below, which will eventually cause the listener to be triggered twice.

Comment From: kriegaex

I looked into this issue too, because the same question was asked on Stack Overflow. Only later I found it here, because I also just wanted to raise an issue for it.

Actually, this not only happens for this rather strange scenario using both self-injection and @Transactional from within the ApplicationListener implementation, but in other cases where listener is being proxied, too, for example if it is the target of a Spring AOP @Aspect. This is a fact, not pure speculation. I tried, and the result is the same.

As for the root cause: In AbstractApplicationEventMulticaster.retrieveApplicationListeners, there are two loops searching for listeners. The first one finds the original listener, the second one finds the bean, which yields a proxy instance.

https://github.com/spring-projects/spring-framework/blob/c9e781676271a980d39063fc1f23a348b0d37be1/spring-context/src/main/java/org/springframework/context/event/AbstractApplicationEventMulticaster.java#L224-L232

This would be relatively easy to remedy, * either by checking allListeners for duplicates after both loops have finished * or by checking for existing non-proxy elements upon inserting in the loop checking registered beans.

Duplicates in this context could be identified by AopProxyUtils.getSingletonTarget(listener).

How about this?

--- a/spring-context/src/main/java/org/springframework/context/event/AbstractApplicationEventMulticaster.java   (revision Staged)
+++ b/spring-context/src/main/java/org/springframework/context/event/AbstractApplicationEventMulticaster.java   (date 1649655053343)
@@ -260,8 +260,9 @@
            for (String listenerBeanName : listenerBeans) {
                try {
                    if (supportsEvent(beanFactory, listenerBeanName, eventType)) {
-                       ApplicationListener<?> listener =
-                               beanFactory.getBean(listenerBeanName, ApplicationListener.class);
+                       ApplicationListener<?> listener = (ApplicationListener<?>) AopProxyUtils.getSingletonTarget(
+                           beanFactory.getBean(listenerBeanName, ApplicationListener.class)
+                       );
                        if (!allListeners.contains(listener) && supportsEvent(listener, eventType, sourceType)) {
                            if (retriever != null) {
                                if (beanFactory.isSingleton(listenerBeanName)) {

This fixes the problem, at least in this context. It might cause problems elsewhere, please verify. If this is OK, would you like to have a PR? If so, for which branch? 5.3.x or main? Maybe I could create a PR for main and a committer could back-port it to all relevant versions which still get updates.

Actually, in the same class there is some proxy unwrapping going on in another place already, seemingly in order to avoid getting unwrapped objects into the list of listeners, which is the opposite of what my patch does. Hmm...

Edit: I just debugged into that code, and it is called indirectly via AbstractBeanFactory.getBean(String) and AbstractAutowireCapableBeanFactory.createBean(String, RootBeanDefinition, Object[]), as you can see here:

Spring Using AOP and DI itself causes Spring ApplicationListener to be fired twice

If you look at the variables in the top stack frame, you see that in AbstractApplicationEventMulticaster.addApplicationListener(ApplicationListener<?>) the method parameter listener is an unwrapped bean, but at the same time the proxy exists already, as can be seen when inspecting the value of listener.eventDemoListener (the auto-wired self-reference). So along the way, somehow the proxy gets lost or is never retrieved. I.e., the logic in addApplicationListener cannot avoid the unwrapped listener to be added to the list, because what it sees is unwrapped already. I.e., my workaround in the patch above to also unwrap the bean is something that might cause duplicate calls in other situations when addApplicationListener really does get called with a proxy instance. Then it would not unwrap it, but in the beans list we would have an unwrapped object due to my patch.

In which way ever this problem is going to be solved, it would be important to either never or always unwrap proxies in order to avoid effective duplicates. To always unwrap would be safe, because unwrapping is always possible, but determining if there is a proxy for the method parameter in addApplicationListener elsewhere is not easily possible. Another workaround would be to make sure that addApplicationListener is never called with an unwrapped object in the first place, if a proxy exists. But how to ensure that, is beyond my knowledge.

Comment From: kriegaex

I think I found a better, more universal solution and created a PR:

https://github.com/spring-projects/spring-framework/blob/1da3022c27a928988758a5f6bc43913f19eccccf/spring-context/src/main/java/org/springframework/context/event/AbstractApplicationEventMulticaster.java#L266-L281

I am confident that this is going to work. There might be a way to avoid the proxy targets (unwrapped objects) in the listeners list in the first place, but this method cannot rely on it, as we can see in this issue. So this more defensively implemented solution should be OK.

Comment From: sbrannen

superseded by PR #28322

Comment From: kriegaex

@sbrannen: How can a bug be superseded by a PR? After the PR will eventually get merged, it ought to close the bug. I think it is bad practice to close a bug before a fix is in place. Maybe you want to reopen for the time being?

Comment From: sbrannen

@sbrannen: How can a bug be superseded by a PR? After the PR will eventually get merged, it ought to close the bug. I think it is bad practice to close a bug before a fix is in place.

That's our policy: in order to keep the number of open "issues" to a minimum, we close an issue as superseded by a PR that addresses the topic.

Maybe you want to reopen for the time being?

If the PR turns out not to address the issue, the original issue can be reopened after the PR is closed.

Comment From: kriegaex

Oh, OK. Thanks for the explanation. Most OSS projects always have pairs of issues and PRs addressing them. Some, like Apache, even enforce it. Spring's policy is rather exotic and kind of counter-intuitive, especially given the fact that my PR contains a comment "fixes [issue id]", which would have closed this issue automatically after a merge. This GitHub feature is also rendered useless by the policy currently in place. But of course, strange as it might seem to me, I am going to stick to it.

If the PR turns out not to address the issue, the original issue can be reopened after the PR is closed.

That is kind of an extra effort, is it not? Anyway, if you prefer it this way, I can live with it.