Spring Framework
  1. Spring Framework
  2. SPR-7856

Application event listeners not removed from listener registry on listener destroy

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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ć

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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 O Donnabhain added a comment -

        This still appears to happen with Spring 3.2.x

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

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

        Show
        Stevo Slavić added a comment - Will convert patch to pull request - to ease merging in the fix.
        Hide
        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 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 added a comment -

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

        Juergen

        Show
        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
            Reporter:
            Stevo Slavić
            Last updater:
            Phil Webb
          • Votes:
            10 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              25 weeks, 1 day ago