Document the need to destroy a bean that was injected via Instance

[nlisker]

As written in the post, it is necessary to manually destroy beans created with Instance#get. The current documentation only says

Provides a fully-constructed and injected instance of T.

I would say that "fully-constructed and injected" gives the impression that it is managed by the container. This will cause unobvious memory leaks. I would suggest adding something along the lines of:

The instance is created in the @Dependent(link to the docs of the annotation) scope not managed directly by the container, Therefor, its lifecycle should be handled by the caller. After using it, it should be released with a call to Instance#destroy (or CDI#destroy). Failing to do so can result in a memory leak (usually when the containing bean goes back to the pool/passivated and is not destroyed).
A typical use case of such an instance is:

Instance<MyBean> creator = ...;
MyBean myBean = creator.get();
myBean.doSomething();
creator.destroy(myBean);

[manovotn]

In the last part of the [paragraph about Instance, it says (among other things):

Applications are encouraged to always call destroy() when they no longer require an instance obtained from Instance.

I am not sure if there are other mentions TBH.

I would say that "fully-constructed and injected" ...

It is exactly what it states - constructed (instantiated by CDI) and having its dependencies injected. And, for the most part, managed. But in some cases it is plain impossible for CDI container to do, see below.

Resolving dependent bean from an Instance basically follows the same rules as any other injection; the dependent bean is bound to the lifecycle of the Instance. So, in most cases you don't even need to manually destroy those obtained dependent instances if the Instance itself belong to a bean that will be eventually destroyed (unless, of course, you go on resolving large quantities of dependent beans). The problematic case is typically with things like CDI.current().getBeanManager().createInstance() which can be obtained from anywhere. This means its dependent beans will never be deleted automagically, until the CDI container shuts down. So it is in fact still managed.

Hope this^ makes sense, feel free to ask further questions :-)

[mkouba]

Provides a fully-constructed and injected instance of T.

This sentence comes from the javax.inject.Provider which is not part of the CDI API. The CDI spec defines what exactly happens if Instance#get() is called. Moreover, it states that "Applications are encouraged to always call destroy() when they no longer require an instance obtained from Instance". In fact, it should read "...call destroy() when they no longer require an instance of a @Dependent bean obtained..." because normal scopes are not affected.

I would say that "fully-constructed and injected" gives the impression that it is managed by the container.

It is managed by the container. However, as mandated by the spec the container must retain all @Dependent instances created via Instance#get() in the creational context of the bean that injected Instance<T>.

That said, it would not hurt to mention this specific behavior in the javadoc of javax.enterprise.inject.Instance<T>.

[nlisker]

The problem is that I see no mention of it either anywhere in the javadocs. Instance doesn't mention it, Provider (from which get is inherited) doesn't mention it, and get itself doesn't mention it. The programmer relies on the Javadoc for correct usage and rarely consults the specs for how to use a class or its methods.

My suggestion is to document whatever is needed to allow programmers to avoid this pitfall, regardless of the exact definition of managed and an out in the form of "we told you so in the specs" :)
You know better when this can occur ("The problematic case is typically with things like...") and why, I just suggested a starting point.

For what it's worth, the Instances I use are obtained by CDI.current().select, but the blog post shows that this can happen when Instance is injected.

[manovotn]

but the blog post shows that this can happen when Instance is injected.

Well, yea. Except it's injected into an EJB bean which isn't by definition a standard CDI bean but I get your point.

The problem is that I see no mention of it either anywhere in the javadocs.

I am not sure how to easily describe it in javadoc because like I said, in many cases, this won't be an issue and won't cause a leak and encouraging users to always destroy instances created from Instance might be an overkill. Especially since it is common that user doesn't necessarily know the scope of the bean they are resolving.
But we could add some note around the lines of:

Dependent beans resolved via Instance have a lifecycle bound to that of the Instance that resolved them. If the Instance is injected into non-contextual object, users are encouraged to destroy any dependent beans created via this Instance when they no longer need them.

(poor wording, I know; it's just the first thing that I thought of :))

[nlisker]

Something like that seems fine. I would just add how to destroy it:

users are encouraged to destroy any dependent beans created via this Instance when they no longer need them by calling Instance#destroy (or CDI#destroy).

Preferably, a short code example similar to what I had in mind can also be added.

I, too, called the CDI.current().select in a @Stateless EJB, so I would say that this issue can be camouflaged well. In fact, I might remind the user of this with something like

If the Instance is injected into a non-contextual object (including EJBs), users are...

[mkouba]

Something like that seems fine. I would just add how to destroy it:

users are encouraged to destroy any dependent beans created via this Instance when they no longer need them by calling Instance#destroy (or CDI#destroy).

Note that a @Dependent bean instance must be destroyed by the same Instance that created it. As described in https://github.com/eclipse-ee4j/cdi/blob/master/api/src/main/java/jakarta/enterprise/inject/Instance.java#L219-L220.

Maybe we should also mention the Instance.Handle which allows users to inspect the scope of the relevant bean and destroy it only if needed.

I, too, called the CDI.current().select in a @stateless EJB.

That's a very bad practice IMO. CDI.current() should only be used if regular injection is not available.

[nlisker]

Note that a @Dependent bean instance must be destroyed by the same Instance that created it.

Ah, then remove the (or CDI#destroy) from my suggestion.

Maybe we should also mention the Instance.Handle which allows users to inspect the scope of the relevant bean and destroy it only if needed.

Looks like a good idea.

That's a very bad practice IMO. CDI.current() should only be used if regular injection is not available.

I agree, but in my case I only know which bean out of many I need during runtime only. I won't be surprised if there's a better pattern to do that.

[nlisker]

@starksm64 In what PR was this fixed? I would like to see the new wording.

[manovotn]

I think this was closed because of the tracking issue under injection API?
I.e. jakartaee/inject#28

I would like to see the new wording.

I wouldn't mind seeing a PR for it :-)

[Ladicek]

I think the issue in AtInject is incorrect, as Instance belongs to CDI, but maybe I got confused?

[manovotn]

Yes, and it boils down to where we want to document it (and I agree Instance looks like your best bet). Technically, it already is specified in the spec, but not mentioned in the javadoc. One idea was the get() method which comes from Provider which is probably the reason for the other issue. The class-level javadoc of Instance might be the go-to spot.

[Ladicek]

Since AtInject's Provider has no way of destroying a provided instance, I don't think we should document the need there. If anything, CDI's Instance can override that method for the purpose of extending the javadoc.