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

Provide shortcut for CDI.current().select(xxx.class).get() #684

Open
tuerker opened this issue Jun 26, 2023 · 12 comments
Open

Provide shortcut for CDI.current().select(xxx.class).get() #684

tuerker opened this issue Jun 26, 2023 · 12 comments

Comments

@tuerker
Copy link

tuerker commented Jun 26, 2023

Would be great to have a shortcut for CDI.current().select(xxx.class).get().

My proposal would be: CDI.get(xxx.class).

I have several hundreds of such calls in my code. Such as shortcut would make the code more concise and thus better readable.

Thanks for consideration.

@Ladicek
Copy link
Contributor

Ladicek commented Jun 26, 2023

If you have hundreds of calls to CDI.current() in your code, you're likely doing something wrong (and performance likely suffers as well). I'd generally assume that you can @Inject Instance<Xxx> for most situations when you need dynamic lookup or just lazy instantiation.

If your application is "half CDI, half not", or something like that, I'd probably advise to build a single bridge instead of bridging ad hoc.

Of course, I have no idea how your application really looks, so above are just a few random thoughts.

And you can always do this:

public class CdiUtil {
    public static <T> T get(Class<T> clazz) {
        return CDI.current().select(clazz).get();
    }
}

@manovotn
Copy link
Contributor

Ladislav is right that CDI.current.get() shouldn't be your go-to hot path for resolving beans.
Standard way for dynamic resolution, assuming you cannot use classic @Inject Foo, is to use @Inject Instance<T>.

CDI.current() is usually a means to access CDI container from places that are not CDI components.

and performance likely suffers as well

This is also good point as by using Instance, implementations can cache and optimize dynamic resolution so that subsequent calls are faster.

@tuerker
Copy link
Author

tuerker commented Jun 26, 2023

Thank you both for your comments. I am accessing stateless service bean methods (running direct database queries) from entity beans. In the entity beans the following does not work:

@Inject ServiceBeanX serviceBeanX;

I am aware of the workaround

public class CdiUtil {
    public static <T> T get(Class<T> clazz) {
        return CDI.current().select(clazz).get();
    }
}

I thought it might useful to all if the class CDI already provides such a shortcut such that it must not be implemented by "everybody".

Maybe, you can help with this question: Is there another way to access service bean methods from entity beans? Thanks in advance.

@manovotn
Copy link
Contributor

Maybe, you can help with this question: Is there another way to access service bean methods from entity beans? Thanks in advance.

Hmm good question, the injection doesn't work because the lifecycle of those objects is not under control of CDI but rather JPA.
I suppose the JPA would have to perform a non-contextual injection for that to work (via Unmanaged API or perhaps just InjectionTarget).

In your case as a user, the dynamic resolution seems like the easiest way unless you'd want to create your own API through which you'd resolve JPA entities and which would attempt injection into those instances. That being said, I am not even sure it's possible, just thinking aloud :)

One more note WRT to simplification of the dynamic resolution.

My proposal would be: CDI.get(xxx.class)

The reason why CDI always let's you go via Instance<T> is, among other things:

  • Because it let's you query the Instance to learn whether that given combination of type and qualifiers is resolvable. Otherwise you'd just get instant exception (unsatisfied/ambiguous dependency)
  • Because any beans with scope @Dependent that you yourself initialize, you should also manually destroy via Instance#destroy(T instance). As dependent beans are normally bound to the lifecycle of the bean they are injected into, in case of dynamic resolution of such bean, you become responsible for that. Failing to do that can have various results from failed cleanup to memory leaks, depending on what those beans do.

So I'm -1 on that simplification :)

@Ladicek
Copy link
Contributor

Ladicek commented Jun 26, 2023

I am accessing stateless service bean methods (running direct database queries) from entity beans. In the entity beans the following does not work:

@Inject ServiceBeanX serviceBeanX;

Okay, that makes sense, entity beans or JPA entities (I sure hope it's the latter :-) ) can't inject anything, because they have a completely different lifecycle, as @manovotn points out.

My proposal would be to do something like this:

@ApplicationScoped
public class ServiceBeanX {
    public static ServiceBeanX get() {
        return CDI.current().select(ServiceBeanX.class).get();
    }

    // ...
}

The get() method may be smarter, it could for example cache the looked up instance in a private static field or something. Or if your services are @Dependent, you could do

@Dependent
public class ServiceBeanX {
    public static void with(Consumer<ServiceBeanX> action) {
        var lookup = CDI.current.select(ServiceBeanX.class);
        var instance = lookup.get();
        try {
            action.accept(instance);
        } finally {
            lookup.destroy(instance);
        }
    }

    // ...
}

And so on. This is how I would centralize lookup and possibly lifecycle handling of those services. The entities would use the static method to obtain the service, while other parts of the application would inject it as usual.

Depending on how many of those service classes exist, what's their lifecycle and how they interact, you might also create a registry of those services and use that to obtain the instances. The only place where you would then use CDI.current() would be to obtain that registry.

@tuerker
Copy link
Author

tuerker commented Jun 26, 2023

@manovotn I cannot really follow the second part of your argumentation. In cases there is no other way than to use

CDI.current().select(clazz).get()

why not to make it easier by having the short-cut notation? This does not harm or change anything.

Maybe, you can help with this question: Is there another way to access service bean methods from entity beans? Thanks in advance.

Hmm good question, the injection doesn't work because the lifecycle of those objects is not under control of CDI but rather JPA. I suppose the JPA would have to perform a non-contextual injection for that to work (via Unmanaged API or perhaps just InjectionTarget).

In your case as a user, the dynamic resolution seems like the easiest way unless you'd want to create your own API through which you'd resolve JPA entities and which would attempt injection into those instances. That being said, I am not even sure it's possible, just thinking aloud :)

One more note WRT to simplification of the dynamic resolution.

My proposal would be: CDI.get(xxx.class)

The reason why CDI always let's you go via Instance<T> is, among other things:

  • Because it let's you query the Instance to learn whether that given combination of type and qualifiers is resolvable. Otherwise you'd just get instant exception (unsatisfied/ambiguous dependency)
  • Because any beans with scope @Dependent that you yourself initialize, you should also manually destroy via Instance#destroy(T instance). As dependent beans are normally bound to the lifecycle of the bean they are injected into, in case of dynamic resolution of such bean, you become responsible for that. Failing to do that can have various results from failed cleanup to memory leaks, depending on what those beans do.

So I'm -1 on that simplification :)

@manovotn I cannot really follow the second part of your argumentation. In cases there is no other way than to use

CDI.current().select(clazz).get()

such as in the JPA entity beans where you cannot use @Inject, why not to make it easier by having the short-cut notation? This does not harm or change anything.

@tuerker
Copy link
Author

tuerker commented Jun 26, 2023

My proposal would be to do something like this:

@ApplicationScoped
public class ServiceBeanX {
    public static ServiceBeanX get() {
        return CDI.current().select(ServiceBeanX.class).get();
    }

    // ...
}

The get() method may be smarter, it could for example cache the looked up instance in a private static field or something. Or if your services are @Dependent, you could do

@Dependent
public class ServiceBeanX {
    public static void with(Consumer<ServiceBeanX> action) {
        var lookup = CDI.current.select(ServiceBeanX.class);
        var instance = lookup.get();
        try {
            action.accept(instance);
        } finally {
            lookup.destroy(instance);
        }
    }

    // ...
}

And so on. This is how I would centralize lookup and possibly lifecycle handling of those services. The entities would use the static method to obtain the service, while other parts of the application would inject it as usual.

Depending on how many of those service classes exist, what's their lifecycle and how they interact, you might also create a registry of those services and use that to obtain the instances. The only place where you would then use CDI.current() would be to obtain that registry.

@Ladicek many thanks for your extensive example. I am seeing what you are trying to achieve. What I dislike is the complexity that is introduced - and thus to be maintained - to gain a tiny performance advantage which is even not recognizable in the web application since the service beans are stateless anyway. They are just created on demand. The garbage collector does the cleaning job sooner or later anyway. Do I overlook something?

I was not expecting that this tiny shortcut feature request is even worth to have a long discussion since it does not harm or change anything in the architecture.

@manovotn
Copy link
Contributor

I was not expecting that this tiny shortcut feature request is even worth to have a long discussion since it does not harm or change anything in the architecture.

Let me show what I meant by examples. Imagine you have the shortcut and can do things like CDI.get(MyBean.class, MyQualifier.Literal.INSTANCE) and further imagine you have the following bean in your app:

@Dependent
public class MyBean {
  // some app logic

  @PreDestroy
  public void cleanup() {
    // this invocation can perform arbitrary app cleanup, DB calls, ...
    // not invoking this method can, in some apps, lead to undefined state or leaks
  }
}

Now, my first point was about making sure the bean is resolvable. Imagine you do this:

CDI.get(Foo.class)

We didn't define Foo bean, so this is an unsatisfied dependency exception and the app immediately crashes.
Whereas with the current approach you can always do:

Instance<Foo> instance = CDI.current().select(Foo.class);
if (instance.isResolvable()) {
 // all good, just return the bean
 return instance.get();
} else {
 // do some logging, return different non-cdi implementation, ....
}

Now the second point - if you manually resolve MyBean:

CDI.get(MyBean.class)

This is will just fine since the bean exists and you can use it.
But when does that @PreDestroy callback get invoked? The answer is, never. Unless you do it manually which would look something like this:

Instance<Foo> instance = CDI.current().select(MyBean.class);
MyBean bean = instance.get();
// do some work with the bean
// once done, you are supposed to destroy the instance if it's @Dependent
// doing so invokes all @PreDestroy invocations and does that for any other dependency that the bean itself might have
instance.destroy(bean);

NOTE: For simplicity sake I've skipped (in the code) bits that would help you determine if the bean is @Dependent scoped.
Ideally, you'd use Instance.Handle for that.

Many beans are often supplies by other dependencies and/or frameworks so assuming you always "know the whole universe" is not safe. Therefore, having the shortcut opens up way for multitudes of misusages and future issues.
Hope I am making sense :)

@tuerker
Copy link
Author

tuerker commented Jun 26, 2023

@manovotn I fully see the both points which makes sense from the global point of view.

Of course, my expection was that CDI.get(Foo.class) throws an unsatisfied dependency exception when the argument (here: Foo.class) does not exist.

My conclusion is slightly different: Whoever so far (mis)used the code fragment

CDI.current().select(MyBean.class).get()

without prior checking whether the instance is resolvable or not, will continue todo that. Actually, it is fine todo so and no misuse at all when all the usage relies on hardcoded classes. In case you use a non-defined class, your compiler will tell it. For such scenarios, the shortcut will make the code more concise and readable.

The second argument that then you cannot do the cleanup is also valid but in case you don't care because you know that the garbage collector will do that job anyway after the invoking object is destroyed, everything is fine again.

Many thanks for your detailed argumentation again. Cheers.

@manovotn
Copy link
Contributor

Of course, my expection was that CDI.get(Foo.class) throws an unsatisfied dependency exception when the argument (here: Foo.class) does not exist.

without prior checking whether the instance is resolvable or not, will continue todo that. Actually, it is fine todo so and no misuse at all when all the usage relies on hardcoded classes. In case you use a non-defined class, your compiler will tell it. For such scenarios, the shortcut will make the code more concise and readable.

The resulting set of beans can be tempered with by 3rd party libraries (or anyone else registering an extension for that matter). Or even by your own user registering a producer for a type + qualifiers that you already have.
Furthermore you can have correct type but wrong qualifiers because all CDI resolution is always type and qualifiers (even if you just specify the type in which case @Default is assumed).

The second argument that then you cannot do the cleanup is also valid but in case you don't care because you know that the garbage collector will do that job anyway after the invoking object is destroyed, everything is fine again.

Not sure I understand, the GC does not invoke your pre-destroy callbacks?

Don't get me wrong, I understand your argument that in a closed world scenario, you can be sure it's safe to use the shortcut.
However, many cases aren't that simple and even as it is today, I've seen some share of issues caused by overusing CDI.current(), hence my scepticism.
I can OFC be outvoted :)

@tuerker
Copy link
Author

tuerker commented Jun 27, 2023

Dear @manovotn i fully see all your points with all the potential issues and really appreciate your detailed explainations.

Don't get me wrong, I understand your argument that in a closed world scenario, you can be sure it's safe to use the shortcut.
However, many cases aren't that simple and even as it is today, I've seen some share of issues caused by overusing CDI.current(), hence my scepticism.

However, I am the optimistic guy. Therefore my standpoint is different: Why should I make the world more difficult than necessary for people who know what they do to prevent others from doing something that might lead to an issue for them, which in any case would easily be solvable. For instance, in our large application with the given requirements, it is safe to use everywhere "CDI.current().select(MyBean.class).get()". Hence, it would great to have such a shortcut to make the code more concise. So, I want to have a short notation for accessing beans from "everywhere"! As already said, it actually is not a big issue since one could go the way with the workaround:

public class CdiUtil {
    public static <T> T get(Class<T> clazz) {
        return CDI.current().select(clazz).get();
    }
}

But then everybody with these same requirements needs to do this additional work which the class CDI could easily take over once for all. Of course, this method does not free anybody who "needs checking whether the bean is resolvable" or "requiring predestroy actions" from their duty to do that over instance CDI.current().select(clazz) to fulfill their requirements.

@mkouba
Copy link
Contributor

mkouba commented Jun 27, 2023

FTR CDI.get(Class<T> type) would not be enough. You would also need CDI.get(TypeLiteral<T> type) to select generic types. Actually, you would need to add the Annotation... qualifiers parameter as well.

In any case, I also don't think it's a good idea to use CDI.current() in the app code, including JPA entities. It was primarily intended for legacy/integration code. It's inefficient and it's definitely not idiomatic.

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