Avoid duplicate ApplicationListener firing (proxy vs. target)

[kriegaex]

In AbstractApplicationEventMulticaster.retrieveApplicationListeners, despite best efforts to avoid it, unwrapped proxies (singleton targets) can end up in the list of programmatically registered listeners. In order to avoid duplicates, we need to find and replace them by their proxy counterparts, because if both a proxy and its target end up in allListeners, listeners will fire twice.

Fixes #28283.

[kriegaex]

Hello. I am not sure who is going to be able to look into this, maybe @jhoeller or @sbrannen? Is there anything I can or need to do in order to facilitate this being reviewed? I know there are many open bugs in Spring, but only maybe 10% of them have actual PRs attached. Maybe there is a way to expedite this and get it off the table. If I just should be waiting because of work overload or Easter holidays, feel free to scold me, I would not mind. At least I would know that this was noticed and I just need to wait. 🙂

[AgataKowal]

Hi @kriegaex
Is there a chance for this PR to be ever merged? Are you aware if this problem has been addressed in any other PRs? It was not easy to find this problem and I was happy to see the solution... I have bumped into the same problem and it causes indexing issues (the event that should always be received only once was received twice). I am wondering how to handle it externally, without changes in the spring core code.

[kriegaex]

@AgataKowal, I am not a committer, just a contributor. Therefore, I have no idea why this is still "in triage" and has never been commented on, not to mention merged. I even directly addressed two developers who previously committed code in this area. There is not much more I can do, only wait and hope.

[sbrannen]

Hi @kriegaex,

Apologies for the belated review. This one slipped through the cracks.

Can you please introduce a test that fails before the change and passes afterwards?

Thanks

[kriegaex]

@sbrannen, I will be travelling a lot in the next few days, but try to see what I can do. It was long ago, but my comments on the related bug should give me enough information to reproduce it. Can you please point me to a directory with similar tests, so I can learn what your usual tests look like? I guess we are talking about an integration test, don't we?

[sbrannen]

Can you please point me to a directory with similar tests, so I can learn what your usual tests look like. I guess we are talking about an integration test, don't we?

Please take a look at org.springframework.context.event.AbstractApplicationEventListenerTests and the other test classes in the same package in the spring-context module. If that doesn't help, let us know.

[kriegaex]

@sbrannen: I just rebased on the main branch locally. Now, IDEA cannot even import the Gradle build anymore. Unfortunately, I am a Maven person and do not speak Gradle. How can I build this thing without JDK 21?

Update: Sigh - never mind, I installed it and somehow managed to make Gradle detect it according to https://github.com/spring-projects/spring-framework/wiki/Build-from-Source#before-you-start. But that was the time box earlier today for actually trying to write a regression test, which still I have not started to do.

[kriegaex]

@sbrannen, unfortunately, I am not an active Spring user, just an AOP expert trying to help where I can. My debugging skills were enough to fix this issue, but I really do not comprehend how those tests work and how I can add my test case to the test bed in the right way. However, I hope that you can.

Please, take a look at temporary commit 294be23. If you run org.springframework.context.event.i28283.DemoApplication from the test directory with VM option -ea or -enableassertions on the JVM command line, the assertion will fail before and pass after the commit fixing the problem. That should give you an idea about where and how to add a regression test to one of the existing classes. Feel free to check out and push to this PR. After the test has been implemented, we can drop the commit containing the reproducer.

I noticed that there are tests checking certain things with CGLIB proxies. Somehow, they would have to be amended or modified to also reproduce the situation with the self-injected event listener.

[kriegaex]

OK, I took some more time to convert my reproducer into a simple test which fails before the fix and passes afterwards, see commit 0dba415. @sbrannen, I think, you can review the PR now.

[kriegaex]

@sbrannen, can we please get this off the table after 1.5 years and before I forget the whole context again and have to think myself into the problem one more time? Like this time, when trying to remember what it was all about, how I debugged it, and spending more time finding out how to write a test for it than fixing the actual bug? I know you must be busy, but please let us put this behind us, and please also port it back to all other versions which still get updates. Thank you.

[sbrannen]

Thanks for adding the failing test. That helps a lot!

[sbrannen]

I took a look at the failing example and the proposed fix.

There's definitely an issue there when an ApplicationListener is proxied and relies on self-injection (via @Autowired in the example MyEventListener).

Note that AbstractApplicationEventMulticaster.addApplicationListener() already contains logic to:

Explicitly remove target for a proxy, if registered already, in order to avoid double invocations of the same listener.

However, that doesn't cover the use case in the provided example. Thus, we need to find the best way to address this.

The proposed fix does indeed allow the example to pass, but I'm not yet sure if the proposed fix is the only/best way to address this issue.

In light of that, someone from the team will need to take a closer look at the underlying issue, tentatively for inclusion in 6.0.12.

[kriegaex]

Thank you for the merge. 🙂

@jhoeller, is anything stopping you from porting back this fix to the 6.0.x and 5.3.x branches? Even in 5.2.x , I saw a commit as recent as July, so maybe that would be a candidate, too. But the former two definitely are, I guess.

Given the fact the the changed code is quite old, I would not expect any serious merge conflicts, if any at all.