Initial specification text for invoker lookups

[Ladicek]

Follows up on #697.

Also CC @Azquelt

[Ladicek]

I also have some initial TCK tests for this. I will add them to https://github.com/jakartaee/cdi-tck/pull/502 tomorrow.

[Ladicek]

I've got 2 open questions about this:

1. I eventually settled on specifying this in terms of obtaining a contextual reference (for the target bean) and obtaining an injectable reference (for arguments, where the corresponding parameters are treated as injection points), instead of performing programmatic lookup. The term lookup in the name of the feature hence seems a little inappropriate. I couldn't figure out a better name, but I know a lot of people here indulge in naming discussions, so perhaps someone will.
2. The way the specification is currently worded, it is not entirely clean whether the arguments array that the caller must pass to Invoker.invoke() must have at least N elements if the target method has N parameters, in case some of the arguments [at the end of the parameter list] are configured for lookup. It seems clear that invoking a method with 5 parameters where the 5th argument is not configured for lookup requires an array with at least 5 elements -- but what if the 5th argument is configured for lookup? An array with 4 elements would clearly suffice, but 1. I'm not sure if the current spec wording allows it, disallows it, or is unclear; 2. I'm not sure if all implementations can actually do this.

I'd be glad to hear any ideas.

[manovotn]

The way the specification is currently worded, it is not entirely clean whether the arguments array that the caller must pass to Invoker.invoke() must have at least N elements if the target method has N parameters, in case some of the arguments [at the end of the parameter list] are configured for lookup. It seems clear that invoking a method with 5 parameters where the 5th argument is not configured for lookup requires an array with at least 5 elements -- but what if the 5th argument is configured for lookup? An array with 4 elements would clearly suffice, but 1. I'm not sure if the current spec wording allows it, disallows it, or is unclear; 2. I'm not sure if all implementations can actually do this.

Current wording suggests that you ignore the elements that have lookup configured and doesn't put any limitations on the size of the array so I'd say right now it's up to the implementation. If we want to be explicit, I'd make it obligatory to pass in an array with at least N elements - you are not allowed to skip any prior element either, even if it has lookup configured.

I eventually settled on specifying this in terms of obtaining a contextual reference (for the target bean) and obtaining an injectable reference (for arguments, where the corresponding parameters are treated as injection points), instead of performing programmatic lookup. The term lookup in the name of the feature hence seems a little inappropriate. I couldn't figure out a better name, but I know a lot of people here indulge in naming discussions, so perhaps someone will.

Personally, I think lookup is still a very good fit - from user's perspective it's pretty self-explanatory. Certainly much more than withInstanceContextualReference() would be ;-)

[Ladicek]

Current wording suggests that you ignore the elements that have lookup configured and doesn't put any limitations on the size of the array so I'd say right now it's up to the implementation.

The previously added specification text for invokers also says:

If the arguments array has fewer elements than the number of parameters of the target method, RuntimeException is thrown.

Now, I think the wording "the invoke() method of the built invoker shall ignore the given element of the arguments array" can possibly be interpreted as a relaxation of the aforementioned rule, so perhaps it's something we should clarify.

Certainly much more than withInstanceContextualReference() would be ;-)

For sure :-) I was thinking maybe injectInstance() and injectArgument(int) would work, but for some reason, it seems weird.

[Azquelt]

Certainly much more than withInstanceContextualReference() would be ;-)

For sure :-) I was thinking maybe injectInstance() and injectArgument(int) would work, but for some reason, it seems weird.

injectArgument(int) seems fine to me but I agree injectInstance() is a bit odd since the instance isn't being injected anywhere.

withContextualInstance() would sound ok (the user wants to use a contextual instance of the bean, rather than providing the instance themselves) but a "contextual instance" is a slightly different thing in the spec.

I think withInstanceLookup() is probably ok, but maybe the Javadoc should give more detail about what will actually happen (i.e. the container will obtain a contextual reference for the bean and call the method on it. If the bean is @Dependent scoped, it will be destroyed after the method returns but before the invoker returns.)

[Azquelt]

Slight tangent: if we specify an injectable reference for each of the injected arguments, does it follow that an injected argument of @Dependent scope can inject an InjectionPoint and retrieve information about the parameter it was injected into?

[Ladicek]

Slight tangent: if we specify an injectable reference for each of the injected arguments, does it follow that an injected argument of @Dependent scope can inject an InjectionPoint and retrieve information about the parameter it was injected into?

That is correct.

I didn't add a test to the TCK for this, though in my tests in ArC, I do have a test where the target method has a parameter of type Instance<Object> and I lookup an InjectionPoint from that. Maybe the scenario you describe is TCK-worthy.

[manovotn]

Slight tangent: if we specify an injectable reference for each of the injected arguments, does it follow that an injected argument of @Dependent scope can inject an InjectionPoint and retrieve information about the parameter it was injected into?

That is correct.

I didn't add a test to the TCK for this, though in my tests in ArC, I do have a test where the target method has a parameter of type Instance<Object> and I lookup an InjectionPoint from that. Maybe the scenario you describe is TCK-worthy.

I don't think this is correct. An arbitrary method parameter isn't an injection point in CDI (i.e. it won't come up in ProcessInjectionPoint observer for instance). Even the javadoc for InjectionPoint explicitly states which items can be injection points and I think it's correct this isn't one as you can but don't need to have injection done for any param of the invoker method. Furthermore, an implementation can choose to implement this functionality by performing a programmatic lookup which is IMO fine. However, the Instance<X> is then the injection point of whatever you resolve with it and as such will show up in your dependent bean.

[Ladicek]

I think I'd really like target method parameters for which a lookup is configured to be injection points, but the inability to fire PIP for them (or observe them from anywhere else in extensions) is not nice.

Need to think more about it.

[Ladicek]

Rebased.

[Ladicek]

Rebased.

Added one commit that avoids turning target method parameters into injection points, per @manovotn's request. (It was easiest for writing the specification, but it is true that these injection points couldn't behave as other injection points in all regards, so probably best to not turn them into injection points in the first place.)

[Ladicek]

Added one more commit (to be squashed) that allows lifecycle extension of @Dependent looked up beans. This is for implementations that feature direct support for asynchronous programming. I expect this might be a topic for discussion :-)

[Azquelt]

I do feel like you're opening up a can of worms with that one...

1. At the moment, having a bean with an indeterminate lifetime isn't something that the spec permits, so this is quite a unique bit of function being used for one niche case. I can see the issue you're trying to solve but the same issue exists if a @Dependent scoped bean ever leaves the class that it's injected into, unless that class is @ApplicationScoped or @Singleton.
2. In particular, the requirement to destroy the bean at a later time (i.e. call @PreDestroy and any interceptors and destroy dependent beans) is unusual. If you were to do a CDI.current().select(MyBean).get() and then never call .destroy(), I would expect the bean to never be destroyed, I would just expect it to eventually be garbage collected.

3. The restriction to only allow this behaviour "when they can conclusively determine that code called by the target method is capable of using the instances after the method returns" is very restrictive. The implication is that if you can't conclusively determine that then you can't extend the lifetime and must destroy the bean when the method returns.

   I assume that you would need to analyse the bytecode to determine that and I'm unsure that you could do so at startup when you may only know the interfaces available and not the types that will implement them at runtime. You may know the exact method that the invoker will call, but if it passes that object to a method on an interface, you don't know what code will be run at that point.

   You could possibly rewrite it to be less restrictive - e.g. that you can extend the lifetime if you can determine that a reference to the object may remain alive after the method returns.

   In general though, I think that determination is still difficult to make and for a CDI Full implementation would have to be done at startup and would impact startup times.

4. I'm not sure about this being something that can be supported in some runtimes but not others. This feature does make it easier to store a dependent-scoped bean indefinitely. If we think that's something that users are going to want to do, then we should require that the lifetime is extended (perhaps even until the object is eligible for garbage collection).

[Ladicek]

Thanks for the comments! I expected you would have something to say :-)

I'll comment on a few things, but I don't consider this a solved problem. I'm ready to eventually drop that part of the specification and solve it on an implementation level. It is however an existing problem that I felt I should tackle in one way or another.

At the moment, having a bean with an indeterminate lifetime isn't something that the spec permits

I don't think the lifetime is indeterminate. The lifecycle extension must be well defined, although that happens outside of the specification. Perhaps that's what you alluded to.

Also, lifecycle of pseudo-scoped beans is not as specifically defined as normal scoped beans, and @Dependent is a pseudo-scope.

In particular, the requirement to destroy the bean at a later time is unusual.

That is correct. It is however usual for @Dependent beans to be instantiated for each invocation of a method and be destroyed when the method returns. What I'm doing here is that for the first time in the CDI specification, I'm admitting the unfortunate fact of life that there are methods that start an action which only finishes long after the method returns. Destroying a @Dependent bean immediately after the method returns renders any usage of such @Dependent bean afterwards an undefined behavior. Either the lifecycle of these @Dependent beans is extended, or invokers for such methods must not configure lookups for @Dependent beans at all. (Or the method body may only use the @Dependent beans "synchronously". This is very easy to get wrong, so I'm not really considering it an option.)

I assume that you would need to analyse the bytecode to determine that

That's not the impression I wanted to make :-) I tried to say that there must be a precise set of conditions defined (and documented) by the implementation, and those conditions must relate to the fact that the code in the method body is expected to use the method parameters even after the method returns. The note below shows that conditioning on the return type of the target method is enough.

Maybe the language I used was too strong. I don't want implementations to prove that the parameters are used by the code in the method body after the method returns; I want them to prove that they may be used in such way.

I'm not sure about this being something that can be supported in some runtimes but not others.

There's a non-trivial amount of non-portable behavior in CDI already. I agree it is not nice to add more, but this behavior is not something I necessary want to inflict on everyone. At the same time, it would be nice if the specification explicitly said this is allowed. If it doesn't, I guess I can live with that.

This feature does make it easier to store a dependent-scoped bean indefinitely.

I don't think that's true. Ignoring invokers completely, there's a decent amount of constructs (even plain injection, but also observers and other container-invoked methods whose parameters are injection points) that allow you very easily to take an instance of a @Dependent bean and store it into a static field of some class. Anyone invoking a method on that instance after it's destroyed is getting into the comfortable territory of undefined behavior.

Also, in the text, I tried to make it very clear that the lifecycle extension must not be infinite. There must be a well-defined point in time where the bean is destroyed. After that, it's undefined behavior again.

[Azquelt]

lifecycle of pseudo-scoped beans is not as specifically defined as normal scoped beans, and @Dependent is a pseudo-scope.

Huh, you're right, we have fairly strict rules for destruction when an @Dependent bean instance is a dependent object, but it's less strict when it isn't. I'd also forgotten about @TransientReference which can be used on objects injected into method or constructor parameters.

I tried to say that there must be a precise set of conditions defined (and documented) by the implementation, and those conditions must relate to the fact that the code in the method body is expected to use the method parameters even after the method returns. The note below shows that conditioning on the return type of the target method is enough.

Ok, I think that makes a bit more sense to me now. However, it seems to me that it's the framework calling the invoker (e.g. the Jakarta REST implementation) that knows whether the parameter will be used after the method returns, rather than the CDI implementation.

Would this functionality be better implemented by including in the Invoker API a way for the caller to control when any dependent beans should be destroyed? That way Jakarta REST can say "this is an async resource method, so I shouldn't destroy the dependent beans until the request completes" and pass a callback to the returned CompletionStage or the AsyncResponse which will destroy any dependent beans?

E.g. something like this:

private <T> CompletionStage<?> callAsyncMethod(Invoker<T, CompetionStage<?>> invoker, T instance, Object[] arguments) {
    InvocationHandle<CompletionStage<?>> handle = invoker.invocationHandle(instance, arguments);
    try {
        CompletionStage<?> result = handle.invoke();
    } catch (Exception e) {
        handle.close();
        throw e;
    }
    return result.whenComplete((r, t) -> handle.close());
}

Potentially we might want to say that the handle is automatically closed if the invocation throws an exception to avoid the caller always needing to cover it with a try-catch.

[Ladicek]

Shifting the responsibility to the caller is a good idea. I'll try to think about how the API for that could look like. One of the options that I can immediately think of is a return value transformer :-)

EDIT: well, not exactly, because a return value transformer doesn't have access to anything that would initiate the dependent beans destruction. But conceptually, it's very close.

Also, thanks for reminding me of @TransientReference, we might potentially want to support that.

[Ladicek]

Just looking back at @TransientReference and it isn't really relevant to our case. @TransientReference is only used for bean constructor / producer method / initializer method parameters to make sure that the @Dependent scoped parameter does not become a dependent object of the bean instance and is instead destroyed after the constructor / method returns.

Destruction after method returns is standard behavior in the other cases that don't "initialize" a bean: observer methods, disposer methods, and now, invokers.

Relevant specification chapters are:

• Dependent objects
• Destruction of objects with scope @Dependent

[manovotn]

E.g. something like this:

private <T> CompletionStage<?> callAsyncMethod(Invoker<T, CompetionStage<?>> invoker, T instance, Object[] arguments) {
    InvocationHandle<CompletionStage<?>> handle = invoker.invocationHandle(instance, arguments);
    try {
        CompletionStage<?> result = handle.invoke();
    } catch (Exception e) {
        handle.close();
        throw e;
    }
    return result.whenComplete((r, t) -> handle.close());
}

Potentially we might want to say that the handle is automatically closed if the invocation throws an exception to avoid the caller always needing to cover it with a try-catch.

I don't see a significant gain versus just resolving dep. beans manually as a caller and that way gaining control over their lifecycle. Plus, I also feel like this might be a can of worms that might not be worth the hassle for 4.1. Given that there is a workaround, I'd say we keep the API minimal and if we figure out this is needed (I suppose ArC might have internal API for this), we can add it in the next version. Just like we settled on transformers being delayed to see if it's actually needed and what shape it should take.

[Azquelt]

I'd say we keep the API minimal and if we figure out this is needed, we can add it in the next version.

I wouldn't mind leaving it until a future release if we can't find a good way to do it. However, we do know that Jakarta REST will need this to use invokers to call async resource methods so, if we can agree on a suitable API, I would prefer to put it in.

It might still be worth merging the rest of this PR first though.

[manovotn]

I'd say we keep the API minimal and if we figure out this is needed, we can add it in the next version.

I wouldn't mind leaving it until a future release if we can't find a good way to do it. However, we do know that Jakarta REST will need this to use invokers to call async resource methods so, if we can agree on a suitable API, I would prefer to put it in.

It might still be worth merging the rest of this PR first though.

Well, in order to destroy a @Dependent instance, you'd need to have a reference to the Instance that created it, or at least its CreationalContext (which is pretty low level). So pretty much the only way (I can think of so far) to achieve this outside of caller handling them is an API where Invoker.invoke() returns you some object that you can later invoke the destruction on. That doesn't seem very helpful as you'd then need to pass this into the async call somehow so that it can eventually get invoked. Plus the caller is responsible for destroying it and handling the success/error case which puts you very close to current state where you might do the same by resolving the deps yourself.

However what we might do is define the destruction in relation to the invoker's target method. So something like:

All instances of @Dependent looked up beans obtained during Invoker.invoke() are destroyed after the invoker's target method returns or throws.

Which would give impls leeway in how they handle it and when they actually destroy it while still indicating that it should be destroyed automatically for you.

[Ladicek]

I eventually figured out that we could use transformers to handle destruction. Suppose we define (intentionally terrible name):

interface InvocationDestroyer {
    void destroy();
}

Then, transformer methods would be permitted to declare an additional parameter of type InvocationDestroyer and if they do, they would get an instance of it. Calling destroy() would destroy all @Dependent objects instantiated for the invocation. Not sure what calling destroy() multiple times would mean (probably just noop?).

We might require an explicit signal for "don't destroy @Dependent beans after the method returns, a transformer will take care of it", but strictly speaking, that wouldn't be necessary, as we could infer that knowledge from the presence of an InvocationDestroyer-accepting transformer.

For example, here are the transformers one would need to write and register for a CompletionStage-returning method, using the previous proposal for transformers:

public static <T> CompletionStage<T> onReturn(CompletionStage<T> result, InvocationDestroyer destroyer) {
    CompletableFuture<T> transformed = new CompletableFuture<>();
    result.whenComplete((value, error) -> {
        destroyer.destroy();
        if (error == null) {
            transformed.complete(value);
        } else {
            transformed.completeExceptionally(error);
        }
    });
    return transformed;
}

public static void onThrow(Throwable exception, InvocationDestroyer destroyer) throws Throwable {
    destroyer.destroy();
    throw exception;
}

To allow actually useful transformers, in addition to these, we would have to support registering multiple transformers for a single output (and probably also input, see below).

Something similar would (I hope :-) ) be possible for JAX-RS AsyncResponse using an argument transformer. The argument transformer would have to wrap the original object and and call InvocationDestroyer in cancel() and resume(), before forwarding these calls to the original. I'm not entirely sure about timeout handling, but I guess it should be doable.

Now, there's a problem with this. People have to know that they need this, they have to write the transformers correctly, and they have to register them correctly. I'm not exactly sure that's a good experience.

There are 2 problems with the specification giving implementations an option to do this under the covers:

1. It cannot be relied upon by specification, obviously.
2. Implementations are not necessarily able to implement deferred destruction for all cases the users might need. For example, implementing it for methods that return CompletionStage is straightforward, implementing it for methods that accept JAX-RS AsyncResponse is not, most importantly because CDI implementations have no reason to depend on the JAX-RS API. So that would at the very least require an implementation SPI.

I think I'm just going to remove the last commit that I added and pretend that this discussion has never happened :laughing:

[Ladicek]

Rebased, squashed the 2nd commit (parameters of target methods are not injection points), removed the 3rd commit (deferred destruction of @Dependent beans).

[Ladicek]

Added one commit to address this:

Now, I think the wording "the invoke() method of the built invoker shall ignore the given element of the arguments array" can possibly be interpreted as a relaxation of the aforementioned rule, so perhaps it's something we should clarify.

To be squashed.

[Ladicek]

Also, here's a proposal for a "last resort" kind of resolution for the problem with destroying @Dependent beans after the target method returns:

Implementations are encouraged, but not required, to destroy all instances of @Dependent looked up beans obtained during Invoker.invoke() after the target method returns or throws. The order in which the instances of @Dependent looked up beans are destroyed is not specified.

NOTE: This will become a requirement in a future version of the specification. It is not a requirement yet because it is currently not clear how to handle "asynchronous" methods (such as methods that return CompletionStage or Jakarta REST resource methods that accept AsyncResponse).

[manovotn]

Also, here's a proposal for a "last resort" kind of resolution for the problem with destroying @Dependent beans after the target method returns:

Implementations are encouraged, but not required, to destroy all instances of @Dependent looked up beans obtained during Invoker.invoke() after the target method returns or throws. The order in which the instances of @Dependent looked up beans are destroyed is not specified. NOTE: This will become a requirement in a future version of the specification. It is not a requirement yet because it is currently not clear how to handle "asynchronous" methods (such as methods that return CompletionStage or Jakarta REST resource methods that accept AsyncResponse).

Hmm, this is good in terms of what it allows implementations to do but it also prevents us from asserting any destruction whatsoever in the TCKs. That might be OK though; I wouldn't mind having this in place.

[Ladicek]

I was also thinking about something like this:

Implementations must destroy all instances of @Dependent looked up beans obtained during Invoker.invoke() after the target method returns or throws. The order in which the instances of @Dependent looked up beans are destroyed is not specified.

This specification recognizes the existence of asynchronous methods, where the action represented by the method does not finish when the method returns; the completion of the action is asynchronous to the method call. Implementations must provide an API that allows integrations to specify which methods shall be considered asynchronous and how to propagate the completion signal to the caller. Other methods are considered synchronous. For synchronous target methods, the instances of @Dependent looked up beans must be destroyed before invoke() returns or throws. For asynchronous target methods, the instances of @Dependent looked up beans must be destroyed after the asynchronous action completes and before the completion is signalled to the caller of invoke(). If an asynchronous target method throws synchronously, the instances of @Dependent looked up beans must be destroyed before invoke() rethrows the exception.

We could add something like InvokerBuilder.setAsynchronous() which the invoker creator should use to signal their expectation. This could be used for validation, perhaps in both ways: 1. if an invoker is marked as asynchronous but the integrator did not provide an "asynchrony handler" for this method, it would be a deployment problem; if an invoker is not marked as asynchronous but an integrator did provide an "asynchrony handler" for this method, that would be a deployment problem too.

But I'm not exactly sure if that's helpful. It feels overspecified and underspecified at the same time :shrug:

[Azquelt]

I think that wording makes sense, though I would change it to:

Implementations may provide an API that allows integrations to specify which methods shall be considered asynchronous and how to propagate the completion signal to the caller.

If we can't come up with a good API for this, I don't think we should require implementations to provide their own.

FWIW, I don't think this text is required to allow this. Implementations can always add additional APIs which extend the behaviour allowed by the spec.

As it was written before, if you were to automatically make an invoker for a method that returns a CompletionStage not destroy dependent objects until after the completion stage completes, you would technically be in violation of the spec. However, if you provide an API or configuration option to enable that behaviour then you're fine.

---

We could add something like InvokerBuilder.setAsynchronous() [...] But I'm not exactly sure if that's helpful. It feels overspecified and underspecified at the same time 🤷

I agree, this doesn't seem helpful.

You'd want the API to be InvokerBuilder.setAsynchronous(AsynchronousInvokerHandler) or InvokerBuilder.setAsynchronous(Class<? extends AsynchronousInvokerHandler>). (I forget whether the former is possible for implementations which process extensions at build time, it's certainly the more useful and flexible.)

Having another think about a possible spec API, would specifying the AsynchronousInvokerHandler like this work? I think it at least covers the Jakarta REST cases.

/**
 * Responsible for handling the destruction of dependent objects for an asynchronous {@link Invoker}.
 * <p>
 * When an invoker calls a method which starts an asynchronous operation, the operation may continue after the method
 * returns and destruction of any dependent objects looked up may need to be deferred until after the operation completes.
 * <p>
 * The {@code AsynchronousInvokerHandler} is called after the method returns and is responsible for ensuring that
 * {@link DependentObjectClosable#close} is called after the asynchronous operation is complete.
 * <p>
 * For example, if a method returns a `CompletionStage` representing the status of the asynchronous operation, then an
 * appropriate {@code AsynchronousInvokerHandler} might be
 * <pre>{@code
 * public class CompletionStageInvokerHandler implements AsynchronousInvokerHandler {
 *     void handleAsynchronousClosure(Object instance, Object[] args, Object result, DependentObjectClosable closable) {
 *         ((CompletionStage<?>) result).handle((r, t) -> closable.close());
 *     }
 * }
 * }</pre>
 */
public interface AsynchronousInvokerHandler {

    /**
     * Calls {@code DependentObjectClosable.close()} to destroy any dependent objects, or delegates responsibility for doing so.
     */
    void handleAsynchronousClosure(Object instance, Object[] args, Object result, DependentObjectClosable closable);

}

Possibly this should use generics for instance and result, though I wasn't confident in writing that in a way that allows for an AsynchronousInvokerHandler which can be used for any instance type or any result type..

[manovotn]

Squashed and merged into main; we can continue the async API discussion here or on a separate issue, whatever is your preference @Ladicek.

[Ladicek]

I think your AsynchronousInvokerHandler is fairly close to what I mentioned above, an invoker transformer or invoker wrapper with an additional parameter for initiating dependent objects destruction.

Maybe that's what it should be in the end. Perhaps with an integration API that allows automatic [conditional] application of transformers/wrappers to all built invokers? Because I feel like if we require users to register something when building the invoker, it's going to lead to subtle bugs (caused by omission).

But I agree with this:

If we can't come up with a good API for this, I don't think we should require implementations to provide their own.

So I'm thinking something like this could work:

This specification recognizes the existence of asynchronous methods, where the action represented by the method does not always finish when the method returns; the completion of the action is asynchronous to the method call. Implementations are permitted (but not required) to provide an API that allows integrators to specify which target methods shall be considered asynchronous. For asynchronous target methods, the requirement to destroy instances of @Dependent looked up beans is relaxed: it is the responsibility of the implementation and the integrator to ensure that instances of @Dependent looked up beans are destroyed properly. It is recommended that the instances of @Dependent looked up beans are destroyed after the asynchronous action completes and before the completion is propagated to the caller of Invoker.invoke(); if an asynchronous target method throws synchronously, it is recommended that the instances of @Dependent looked up beans are destroyed before invoke() rethrows the exception.

NOTE: This optional behavior may become a requirement in a future version of this specification.