Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
gwenneg opened this issue Feb 29, 2024 · 6 comments
Assignees

Comments

@gwenneg
Copy link
Contributor

gwenneg commented Feb 29, 2024

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.

@andrejpetras
Copy link
Member

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

@gwenneg
Copy link
Contributor Author

gwenneg commented Mar 1, 2024

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 gwenneg self-assigned this Mar 5, 2024
@gwenneg
Copy link
Contributor Author

gwenneg commented Mar 6, 2024

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... 😅

@mkouba
Copy link

mkouba commented Mar 6, 2024

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

@gwenneg
Copy link
Contributor Author

gwenneg commented Mar 6, 2024

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

Awesome 🎉

Thanks for the detailed explanation!

@manovotn
Copy link

manovotn commented Mar 6, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants