Application event listeners not removed from listener registry on listener destroy [SPR-7856]

[spring-projects-issues]

Stevo Slavić opened SPR-7856 and commented

See Spring Forum Reference, for more details and app that reproduces the issue.

---

Affects: 3.0.5

Reference URL: http://forum.springsource.org/showthread.php?t=97899

Attachments:

• SPR-7856.patch (2.97 kB)
• SPR-7856-2.patch (3.50 kB)

Issue Links:

• Inner bean behind BeanFactoryPostProcessor should be able to receive application events [SPR-14783] #19349 Inner bean behind BeanFactoryPostProcessor should be able to receive application events

Referenced from: commits 0fe4962

10 votes, 6 watchers

[spring-projects-issues]

Jacob Zwiers commented

This issue is the result of the following commits:

1. Revision 607 (https://fisheye.springsource.org/changelog/spring-framework?cs=607) in which AbstractApplicationEventMulticaster.getApplicationListeners() was modified to look to a bean factory to lookup (creating if necessary) ApplicationListener instances
2. Revision 646 (https://fisheye.springsource.org/changelog/spring-framework?cs=646) in which AbstractApplicationEventMulticaster.getApplicationListeners(ApplicationEvent) method is created and then SimpleApplicationEventMulticaster.multicastEvent(ApplicationEvent) calls that method instead of the no-arg getApplicationListeners().

In theory, the first alone would have introduced this behaviour. However, if the no-arg call to getApplicationListeners() had been retained (not saying this is correct), we would not see this issue.

These lines of code in AbstractApplicationEventMulticaster.getApplicationListeners(ApplicationEvent event) causes the issue:

if (!this.defaultRetriever.applicationListenerBeans.isEmpty()) {
  BeanFactory beanFactory = getBeanFactory();
  for (String listenerBeanName : this.defaultRetriever.applicationListenerBeans) {
    ApplicationListener listener = beanFactory.getBean(listenerBeanName, ApplicationListener.class);
    if (!allListeners.contains(listener) && supportsEvent(listener, eventType, sourceType)) {
      retriever.applicationListenerBeans.add(listenerBeanName);
      allListeners.add(listener);
    }
  }
}

By calling beanFactory.getBean() without any checks against state, etc, the factory attempts to load beans which have already been "un-loaded" by the a prior processing of the ApplicationEvent.

[spring-projects-issues]

Stevo Slavić commented

Attached [^SPR-7856.patch]. It fixes this issue by removing appropriate bean definition when singleton beans get destroyed. I guess it was design decision not to do this - maybe there is a reason for this; anyway, all existing tests pass with the fix applied. I wish spring reference docs contained more details on container destruction phase.

Destroying listener bean, with this patch applied, still doesn't remove listener from another registry - applicationListenerBeans of AbstractApplicationEventMulticaster's ListenerRetriever, because multicaster is not available from AbstractBeanFactory, multicaster is in AbstractApplicationContext. As workaround for this, patch adjusts AbstractApplicationEventMulticaster to skip retrieving (creating) listener bean from BeanFactory if BeanFactory doesn't contain listener bean.

Maybe a different fix could be to make a DestructionAwareBeanPostProcessor which would implement ApplicationContextAware and for any context which implements AbstractApplicationContext and for any bean which implements ApplicationListener call removeApplicationListener and removeApplicationListenerBean on AbstractApplicationContext's ApplicationEventMulticaster. Container would have to enable that DestructionAwareBeanPostProcessor always or by default.

[spring-projects-issues]

Stevo Slavić commented

Here is another patch [^SPR-7856-2.patch] which fixes this issue by adding ApplicationListenerDestructionAwareBeanPostProcessor which on (just before) destroy of ApplicationListener beans unregisters them from ApplicationEventMulticaster. All existing tests pass.

[spring-projects-issues]

Felix Martin commented

As a workaround you can register your own DestructionAwareBeanPostProcessor following the example in the second patch. More details in the Spring Forum Reference

[spring-projects-issues]

Donnchadh O Donnabhain commented

This still appears to happen with Spring 3.2.x

[spring-projects-issues]

Stevo Slavić commented

Will convert patch to pull request - to ease merging in the fix.

[spring-projects-issues]

Juergen Hoeller commented

I cannot reproduce the behavior reported in the forum thread - that is, not in any form that actually logs a warning on shutdown or does other things that may irritate the casual observer. Any singleton ApplicationListener bean referred to by bean name seems to be cached as an instance in the ListenerRetriever (at the first time it receives an event) before we ever try to publish to it on shutdown... so we never actually fail with a bean lookup exception. For that reason, I don't consider this critical on 3.x.

That said, it would of course be correct to unregister each listener as it gets destroyed, for the case where events still get published during the shutdown phase. At the moment we may invoke listener beans after their own destroy method has been called already... which may lead to an unexpected invocation against cleared state in that listener instance. The correct behavior would be to completely ignore a listener once its instance got destroyed in the bean factory. As of 4.0, we're going to do that properly.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

AbstractApplicationContext's ApplicationListenerDetector removes listeners from the ApplicationEventMulticaster on individual destruction of each listener now.

Juergen