Memory leak - WELD's optimisation is not being applied.

[chrisjr]

Hi,

I've recently discovered a memory leak in my application (sample project is attached). Now I am aware that when Instance<>.get() creates a @Dependent scoped bean, the CDI spec states that the bean "belongs" to the Instance<> and should therefore be destroyed afterwards using Instance<>.destroy(). However, [WELD-1076] implemented an optimisation whereby the Instance<> would not keep a contextual reference for the bean provided the bean did not implement a @PreDestroy method or a @Disposes method, or was itself dependent on a @Dependent bean with such a requirement.

In my example application, I have two @Dependent beans called Messenger and Ping. Neither bean has a @PreDestroy method. However, Messenger has a dependency on Instance<Ping>.

• Every GET /ping request invokes Instance<Ping>.get(), and the Instance correctly does not keep a contextual reference for the Ping bean .
• Every GET /leak request invokes Instance<Messenger>.get(). However, in this case the Instance does keep a contextual reference for the Messenger bean .

So Messenger's Instance<Ping> dependency is preventing WELD from optimising away the contextual reference for each Messenger bean, despite the fact that Instance<Ping> will never contain any contextual references for Ping beans that will need destroying.

I would argue that if WELD can apply its optimisation to a bean then it can also apply that optimisation to Instances of that bean. Does this sound reasonable, please?

Cheers,

Chris

[mkouba]

Hi Chris,

optimization for built-in Instance is not possible - see also my comment on WELD-1996 and let me know if it makes sense.

[chrisjr]

Umm, you presumably mean this comment:

no - "An instance of a bean with scope @Dependent obtained by direct invocation of an Instance is a dependent object of the instance of Instance."

As I understand it, this means that the optimisation cannot be applied to Instance beans in the general case. However, in my specific case, the optimisation is already being applied to the Ping bean, which in turn means that Ping beans are not dependent objects of the Instance<Ping> bean that created them. So wouldn't that mean that the optimisation can also be applied to the Instance<Ping> bean? That would then allow the optimisation to be applied to Messenger and Instance<Messenger> beans too, thus removing the memory leak.

[mkouba]

I see what you mean but I think this would complicate the optimization algorithm a lot. For instance, we could detect that Instance<Ping> resolves to a single optimizable bean Ping, but we would also have to check, whether it's not possible to select a child Instance for a subtype of Ping (and this check is neither trivial nor inexpensive). I'm not sure it's worth it.

[chrisjr]

Ah yes. As the application writer I can easily see that my Ping beans are optimisable and have no descendants, of course.

Would it help if the Ping class were also declared as final ... ? After all, it's a @Dependent scoped bean and so you don't necessarily need to create a proxy for it.

However, having said that, the Ping bean is representing CloseableHttpClient in my real application (instantiated via a @Produces method without a matching @Disposes method). So triggering special behaviour for final classes wouldn't help me there. I guess I'd need a annotation like @JustOptimiseAnywayAndAcceptConsequences really.

[mkouba]

Might help but I find it quite cryptic. Anyway, the purpose of this optimization is to "minimize the damage", not to replace the need for Instance.destroy(). It's also not portable. So users are encouraged to use Instance.destroy() where possible. If you're not comfortable with calling the destroy method, you could create a helper class similar to AutoDestroyable I'm describing on the cdi-dev ML: http://lists.jboss.org/pipermail/cdi-dev/2016-May/008241.html

[chrisjr]

Martin Kouba wrote:

Anyway, the purpose of this optimization is to "minimize the damage", not to replace the need for Instance.destroy(). It's also not portable. So users are encouraged to use Instance.destroy() where possible.

I'm actually using WELD here to create Runnable objects for a ThreadPool, e.g. Instance<Runnable>.get(). And one of the Runnable's dependencies happened to be an Instance<CloseableHttpClient> bean.

As it happens, I have discovered that I only need a single CloseableHttpClient and so I don't need the second Instance<> bean after all. So I've already resolved the memory leak. But the underlying cause was really subtle and it caught me completely unawares.

Having said that, I still think that I'd have tied myself up in knots trying to execute destroy() for the Runnable at the right time. (And also from the correct thread?)

[mkouba]

Well, your use case seems to be a bit specific and I'm afraid I can't help without the deeper knowledge of the source. But if you don't control the execution of a Runnable it might be really tricky.

[chrisjr]

Martin Kouba wrote:

But if you don't control the execution of a Runnable it might be really tricky.

It's a "background task", so the idea is to "weld" the Runnable together and then leave it alone to do its thing. Which works best if I don't also have to bury its body when it finally dies.

Which is where that optimisation comes in...

[mkouba]

Yes, I understand. Maybe you could use non-contextual instances here, see also 11.3.4. Obtaining non-contextual instance.

[chrisjr]

Maybe you could use non-contextual instances here, see also 11.3.4. Obtaining non-contextual instance.

Hmm, interesting - I will need to examine this idea further. However, it also occurs to me that if my Runnable bean is suitable for the WELD optimisation (i.e. no @Dependent dependencies with @PreDestroy or @Disposes methods etc) then Instance<>.destroy() should be a no-op anyway . So why not just invoke destroy() before dispatching the Runnable to the ThreadPool? Would that be a safe / portable(ish) thing to do, even if semantically dirty? Or would that also trash any built-in dependent beans such as Instance or BeanManager?

[mkouba]

Theoretically, this should work for you. But I'd rather avoid doing this if possible. You could also use BeanManager.getReference() to produce a dependent bean which is not tracked anywhere.

[chrisjr]

You could also use BeanManager.getReference() to produce a dependent bean which is not tracked anywhere.

Heh, that reminds me of "ye olde code" that I wrote for WELD 1.1.2 when I had a similar memory leak problem in JBoss 6.1:

Bean<?> bean = beanManager.resolve(beanManager.getBeans(type));
CreationalContext<?> context = beanManager.createCreationalContext(bean);
try {
    return (T) beanManager.getReference(bean, bean.getBeanClass(), context);
} finally {
    context.release();
}

Except that context.release() also triggered the destruction of beans with @PreDestroy and @Disposes methods, which made it similar to calling Instance<>.destroy(). (WELD 1.1.2 predated the existence of Instance<>.destroy(), of course).

[chrisjr]

I'd much rather encapsulate the low-level BeanManager code rather than embed it directly in my application, and so I've ended up with this:

@Dependent
class MyThreadProducer {

    @Unmanaged // my new qualifier
    @Produces
    MyThread create(@TransientReference BeanManager beanManager) {
        // Invoke a static function that creates my bean using low-level CDI methods.
        return unmanaged(beanManager, MyThread.class);
    }

}

And then I can create instances of MyThread using:

@Unmanaged
@Inject
private Provider<MyThread> provider;

MyThread myThread = provider.get();

This does work as expected, but probably only because WELD is now always able to optimise away the contextual reference for MyThread regardless of which dependencies it may have.

[mkouba]

Yes, this works because MyThreadProducer.create() is a dependent bean (producer method) with no @Dependent dependencies (due to @TransientReference).