11 Replies Latest reply on Oct 2, 2015 5:56 AM by mkouba

    Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy

    joshuak

      To dynamically inject instances of an interface, we are using code similar to the following:

       

      @ApplicationScoped
      public class SomeCDIBean
      {
          @Inject @Any
          private Instance<SomeInterface> interfaceInstance;
      
          public void someMethod()
          {
                SomeInterface instance = interfaceInstance.select(new Qualifier("someValue")).get();
                instance.doSomething();
                interfaceInstance.destroy(instance);
          }
      }
      
      
      

       

      Implementations of SomeInterface can be either @Dependent or @ApplicationScoped. In order to prevent a memory leak destroy() is called on the dynamically injected instances of SomeInterface. This works as expected for @Dependent beans, however, with @ApplicationScoped beans, this destroys the actual bean itself instead of just the proxy, causing future calls against that specific instance to be in an incorrect state.

       

      After some research I found the following in the org.jboss.weld.bean.builtin.InstanceImpl.destroy() method:

       

      // check if this is a proxy of a normal-scoped bean
              if (instance instanceof ProxyObject) {
                  ProxyObject proxy = (ProxyObject) instance;
                  if (proxy.getHandler() instanceof ProxyMethodHandler) {
                      ProxyMethodHandler handler = (ProxyMethodHandler) proxy.getHandler();
                      Bean<?> bean = handler.getBean();
                      Context context = getBeanManager().getContext(bean.getScope());
                      if (context instanceof AlterableContext) {
                          AlterableContext alterableContext = (AlterableContext) context;
                          alterableContext.destroy(bean);
                          return;
                      } else {
                          throw BeanLogger.LOG.destroyUnsupported(context);
                      }
                  }
              }
      
      
      

       

      Along with this comment in the org.jboss.weld.bean.ContextualInstanceStrategy class:


      /**
      * ...
      * For {@link ApplicationScoped} beans a special strategy is used which caches application-scoped bean instances in a volatile field. This implementation respects
      * the possibility of an instance being destroyed via {@link AlterableContext} and the cached instance is flushed in such case.
      * ...
      */
      
      
      

       

      This seems counterintuitive to the abstraction of Instance. I would expect that as the call to Instance.get() didn't necessarily create the @ApplicationScoped bean but created the proxy, the call to Instance.destroy() wouldn't destroy the bean but would destroy the proxy. Is this a bug or as designed?

        • 1. Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
          mkouba

          Hi joshuak,

          Instance.destroy() is intended to destroy bean instances, not proxies. See also 5.6.1. The Instance interface and 6.2. The Context interface.

          I would expect that as the call to Instance.get() didn't necessarily create the @ApplicationScoped bean but created the proxy, the call to Instance.destroy() wouldn't destroy the bean but would destroy the proxy.

          Yes, for normal-scoped bean you get a client proxy and the bean instance is not necessarily created. In this case, Instance.destroy() should be a no-op. However, you're right that destoying an existing @ApplicationScoped bean instance might sometimes cause problems. And it's also true that there is no way to obtain this info (e.g. the scope of the bean) from the Instance. I will create a spec issue.

          • 2. Re: Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
            joshuak

            Thank you for directing to the sections of the spec that address this matter.

             

            So, if I'm understanding you correctly, the intended functionality is that calling Instance.destroy() on normal-scoped beans should be a no-op, however this is ambiguous in the spec, which leads to the bug I am experiencing in the implementation. From my perspective, this leads to a more general problem where Instance.destroy() has different semantics than AlterableContext.destroy(). No matter what the scope (normal or custom), the former seems to mean release, letting the scope hold onto the instance if that is its function (e.g. @ApplicationScoped), while the latter actually means destroy, disregarding the scope's function. Would you agree with this interpretation?

             

            Also, can link me to the issue you created?

             

            As an aside, how do you format class/method names that way?

            • 3. Re: Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
              mkouba

              So, if I'm understanding you correctly, the intended functionality is that calling Instance.destroy() on normal-scoped beans should be a no-op...

              No. It's a no-op if a bean instance was not created yet. Weld is creating bean instances lazily,  i.e. a bean instance is not initialized until a proxy method is invoked.

              Also, can link me to the issue you created?

              https://issues.jboss.org/browse/CDI-515

              As an aside, how do you format class/method names that way?

              Just switch to HTML mode and put the method name inside the <code> element.

              • 4. Re: Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
                chrisjr

                I've noticed that this issue still exists in WELD 2.2.16.SP1, which means that I can't call Instance.destroy() for any @ApplicationScoped beans obtained this way. Does this mean that the Instance<> will slowly accumulate bean proxies over time, e.g. if this Instance<> is itself a field on an @ApplicationScoped bean?

                 

                Cheers,

                Chris

                • 5. Re: Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
                  mkouba

                  Hi Chris, what issue do you mean? I believe there is no Weld issue, i.e. the behavior is expected and aligns with the spec.

                  • 6. Re: Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
                    chrisjr

                    I mean that if you have (say)

                    @Inject
                    @Any
                    private Instance<MyBean> instance;
                    
                    ...
                    
                    {
                        MyBean bean = instance.get();
                        ...
                        instance.destroy(bean);
                    }
                    

                     

                    where MyBean happens to be @ApplicationScoped, then MyBean is actually destroyed by instance.destroy() as if it were a @Dependent scoped bean. Basically, I would expect an @ApplicationScoped bean to survive until the application terminates, regardless of whether or not someone has acquired a reference for it programmatically. The only thing I would expect instance.destroy() to do here would be to ensure that the proxy was cleaned up correctly and removed from the Instance<> field.


                    But if WELD's current behaviour is correct, then consider the case of an application with (say) three @ApplicationScoped beans DoThis, DoThat and DoOther. Each request will invoke precisely one of these beans, and will obtain a reference to the correct bean via  @Any Instance<>.select().get(). For the sake of argument, this Instance<> also happens to be a field on another @ApplicationScoped "multiplexing" bean which prevents it from being destroyed when each request ends. So given that we can't invoke Instance<>.destroy() because it would destroy an @ApplicationScoped bean that is wanted by other requests, is this going to leak memory?


                    Cheers,

                    Chris

                    • 7. Re: Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
                      mkouba

                      where MyBean happens to be @ApplicationScoped, then MyBean is actually destroyed by instance.destroy() as if it were a @Dependent scoped bean. Basically, I would expect an @ApplicationScoped bean to survive until the application terminates, regardless of whether or not someone has acquired a reference for it programmatically. The only thing I would expect instance.destroy() to do here would be to ensure that the proxy was cleaned up correctly and removed from the Instance<> field.

                      Ok, once again the Instance.destroy() method is intended to destroy contextual bean instances. Note that Instance.get() does not return a contextual instance, but a contextual reference (client proxy for normal-scoped beans). So if you call the destroy method, the contextual instance is destroyed and removed from the application context. The Instance object (as you say "field") does not reference the contextual instance or the contextual reference (client proxy) directly. So for your case - I'm not really sure I completely understand but I would say it would not leak memory because there will be nothing preventing DoThis, DoThat and DoOther bean instances from being garbage collected. And client proxy instances are shared in Weld.

                       

                      In any case, it's definitely a bad idea to destroy @ApplicationScoped bean instances like this. You should probably rather use @RequestScoped.

                      • 8. Re: Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
                        chrisjr

                        In any case, it's definitely a bad idea to destroy @ApplicationScoped bean instances like this. You should probably rather use @RequestScoped.

                        The problem is that you potentially do get a memory leak if the beans are @Dependent and you don't call Instance<>.destroy() after Instance<>.get(). (WELD's optimisation for beans without @Disposes or @PreDestroy methods notwithstanding). And I would expect the usage semantics of Instance<> always to be the same, regardless of the scopes of the beans involved.

                         

                        Maybe we need a new Instance<>.release() method that is guaranteed to do "The Right Thing" for both @ApplicationScoped and @Dependent beans, i.e. "undo" whatever Instance<>.get() did?

                        • 9. Re: Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
                          mkouba

                          Chris Rankin napsal(a):

                           

                          In any case, it's definitely a bad idea to destroy @ApplicationScoped bean instances like this. You should probably rather use @RequestScoped.

                          The problem is that you potentially do get a memory leak if the beans are @Dependent and you don't call Instance<>.destroy() after Instance<>.get(). (WELD's optimisation for beans without @Disposes or @PreDestroy methods notwithstanding). And I would expect the usage semantics of Instance<> always to be the same, regardless of the scopes of the beans involved.

                           

                          Maybe we need a new Instance<>.release() method that is guaranteed to do "The Right Thing" for both @ApplicationScoped and @Dependent beans, i.e. "undo" whatever Instance<>.get() did?

                          Sorry but I still don't understand what the "right thing" should be. For @Dependent beans it's sometimes reasonable to use Instance.destroy() and I believe this was the main motivation to add this feature. I'm not so sure it makes sense for normal scopes. But in fact, the semantics is the same for both - destroy a contextual instance (if it exists). Anyway, feel free to create a spec issue and explain your needs to the EG: CDI Specification Issues - JBoss Issue Tracker

                          • 10. Re: Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
                            chrisjr

                            For @Dependent beans it's sometimes reasonable to use Instance.destroy() and I believe this was the main motivation to add this feature. I'm not so sure it makes sense for normal scopes.

                            And that's the problem: the correct usage semantics of Instance<> are different, depending upon the scopes of the target beans. In my opinion, forcing the Instance's user to care how the target beans are implemented violates encapsulation.

                             

                            Is it unreasonable to expect Instance<> to support some usage pattern that will always work, regardless of the beans' scopes?

                            • 11. Re: Re: Instance.destroy() actually destroys the underlying bean for @ApplicationScoped instead of the proxy
                              mkouba

                              Well, it depends how you define "always work". From my point of view it always destroys the contextual instance. But as I said before you can raise a spec issue.