Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-7856

Application event listeners not removed from listener registry on listener destroy

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Complete
    • Affects Version/s: 3.0.5
    • Fix Version/s: 4.0 RC1
    • Component/s: Core
    • Labels:
      None

      Description

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

      1. SPR-7856.patch
        3 kB
        Stevo Slavić
      2. SPR-7856-2.patch
        3 kB
        Stevo Slavić

        Issue Links

          Activity

          Hide
          intellectual_tortoise Jacob Zwiers added a comment -

          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.

          Show
          intellectual_tortoise Jacob Zwiers added a comment - 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 .
          Hide
          sslavic Stevo Slavić added a comment -

          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.

          Show
          sslavic Stevo Slavić added a comment - 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.
          Hide
          sslavic Stevo Slavić added a comment -

          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.

          Show
          sslavic Stevo Slavić added a comment - 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.
          Hide
          fmarmar Felix Martin added a comment -

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

          Show
          fmarmar Felix Martin added a comment - As a workaround you can register your own DestructionAwareBeanPostProcessor following the example in the second patch. More details in the Spring Forum Reference
          Hide
          donnchadh Donnchadh O Donnabhain added a comment -

          This still appears to happen with Spring 3.2.x

          Show
          donnchadh Donnchadh O Donnabhain added a comment - This still appears to happen with Spring 3.2.x
          Hide
          sslavic Stevo Slavić added a comment -

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

          Show
          sslavic Stevo Slavić added a comment - Will convert patch to pull request - to ease merging in the fix.
          Hide
          juergen.hoeller Juergen Hoeller added a comment -

          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

          Show
          juergen.hoeller Juergen Hoeller added a comment - 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
          Hide
          juergen.hoeller Juergen Hoeller added a comment -

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

          Juergen

          Show
          juergen.hoeller Juergen Hoeller added a comment - AbstractApplicationContext's ApplicationListenerDetector removes listeners from the ApplicationEventMulticaster on individual destruction of each listener now. Juergen

            People

            • Assignee:
              juergen.hoeller Juergen Hoeller
              Reporter:
              sslavic Stevo Slavić
              Last updater:
              Juergen Hoeller
            • Votes:
              10 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                3 years, 5 weeks, 4 days ago