quarkus-unleash icon indicating copy to clipboard operation
quarkus-unleash copied to clipboard

Memory leak risk with `@FeatureToggle Instance<Boolean>` and `@FeatureVariant Instance<?>`

Open gwenneg opened this issue 1 year ago • 6 comments

From what I saw in the extension code, when @FeatureToggle Instance<Boolean> or @FeatureVariant Instance<?> are used, the injected bean doesn't have an explicit CDI scope which means it defaults to @Dependent.

When a @Dependent bean is injected with Instance<?> into an @ApplicationScoped or @Singleton bean and is no destroyed when it's no longer needed, it stays in the memory forever, so there's basically a memory leak.

Is there a risk of memory leak with this extension, or did I miss something? If there is a risk of leak, the doc should at least explain how to prevent it and show how the injected bean can be destroyed.

gwenneg avatar Feb 29 '24 14:02 gwenneg

@gwenneg thank you for checking the implementation. Could we change the implementation to avoid the memory leak?

andrejpetras avatar Feb 29 '24 20:02 andrejpetras

If the leak is confirmed (which I'll try to check next week), there are several ways to "fix" it.

One of them would be a simple doc update, showing how to destroy the Boolean field after it's been used:

try (Instance.Handle<Boolean> toggleHandle = toggle.getHandle()) {
    boolean toggleValue = toggleHandle.get();
    // do something with toggleValue
}

There are other approaches such as using a wrapper class for the toggle field. But before considering any changes, the leak has to be confirmed 😄

gwenneg avatar Mar 01 '24 14:03 gwenneg

Hey @mkouba!

Is there something in Arc that prevents memory leaks when a @Dependent bean is injected into an @ApplicationScoped bean with Instance<?> and not destroyed afterwards?

public class FooProducer {

    @Produces
    @Dependent
    Foo produce() {
        return new Foo();
    }
}

@ApplicationScoped
public class LongLivedBean {

    @Inject
    Instance<Foo> foos;

    public void doSomething() {
        Foo foo = foos.get();
        // use foo without calling foos.destroy(foo) eventually
    }
}

I expected a potential memory leak in this extension but my profiler showed a healthy memory so I may be missing something... 😅

gwenneg avatar Mar 06 '24 09:03 gwenneg

I expected a potential memory leak in this extension but my profiler showed a healthy memory so I may be missing something... 😅

So both Weld and ArC try to mitigate this problem in a way that only some references to @Dependent beans are stored in the CreationalContext.

In ArC, we only add a @Dependent bean to the CreationalContext if (1) a class bean has @PreDestroy interceptor or @PreDestroy callback, (2) a producer has a disposal method and (3) a synthetic bean has some destruction logic OR if the bean has a @Dependent dependency that matches the rules (1), (2) or (3).

In your case, there is a producer with no disposer and no @Dependent depencency == no leak ;-). But of course it's always safer to destroy the bean after use.

CC @manovotn

mkouba avatar Mar 06 '24 13:03 mkouba

In your case, there is a producer with no disposer and no @Dependent depencency == no leak ;-)

Awesome 🎉

Thanks for the detailed explanation!

gwenneg avatar Mar 06 '24 13:03 gwenneg

I expected a potential memory leak in this extension but my profiler showed a healthy memory so I may be missing something... 😅

So both Weld and ArC try to mitigate this problem in a way that only some references to @Dependent beans are stored in the CreationalContext.

In ArC, we only add a @Dependent bean to the CreationalContext if (1) a class bean has @PreDestroy interceptor or @PreDestroy callback, (2) a producer has a disposal method and (3) a synthetic bean has some destruction logic OR if the bean has a @Dependent dependency that matches the rules (1), (2) or (3).

In your case, there is a producer with no disposer and no @Dependent depencency == no leak ;-). But of course it's always safer to destroy the bean after use.

CC @manovotn

Yea, I can confirm this is the case even for Weld (code here). This is both, a workaround to reduce a chance of memory leak because of bad handling and an optimization as there is no real need to keep the reference if the destruction is a no-op.

That being said, you should handle the bean properly and destroy it when no longer needed (assuming it is intended that those beans are dependent and repeated invocations generate new instances OFC).

manovotn avatar Mar 06 '24 14:03 manovotn