-
Notifications
You must be signed in to change notification settings - Fork 77
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
Initial specification text for invoker lookups #749
Conversation
I also have some initial TCK tests for this. I will add them to jakartaee/cdi-tck#502 tomorrow. |
I've got 2 open questions about this:
I'd be glad to hear any ideas. |
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.
Personally, I think lookup is still a very good fit - from user's perspective it's pretty self-explanatory. Certainly much more than |
The previously added specification text for invokers also says:
Now, I think the wording "the
For sure :-) I was thinking maybe |
I think |
Slight tangent: if we specify an injectable reference for each of the injected arguments, does it follow that an injected argument of |
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 |
When `withInstanceLookup()` is called on an invoker builder and the target method is not `static`, the `invoke()` method of the built invoker shall ignore the `instance` argument and instead obtain and use a contextual reference for the target bean and the bean type that declares the target method. | ||
Calling `withInstanceLookup()` on an invoker builder for a `static` target method has no effect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it not throw an exception in this case?
This did also make me question why we would restrict invokers to only work on managed beans, but allow them to work on static methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can have static
producer methods, disposer methods and observer methods, so it seems natural to allow invokers for static
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether withInstanceLookup()
for a static
method should throw an exception, I'm not sure. I guess I don't really have an opinion on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I had completely forgotten that producer and observer methods can be static.
With that it mind, it would be reasonable for someone to say "I want to call this method. If it's not static, I want to call it on a contextual reference" and I feel happier about withInstanceLookup
having no effect if the method is static
.
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 |
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. |
b44e33f
to
250c289
Compare
Rebased. |
250c289
to
c366a90
Compare
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.) |
Added one more commit (to be squashed) that allows lifecycle extension of |
I do feel like you're opening up a can of worms with that one...
|
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.
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
That is correct. It is however usual for
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.
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.
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 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. |
Huh, you're right, we have fairly strict rules for destruction when an
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 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. |
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 |
Just looking back at 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: |
I don't see a significant gain versus just resolving dep. beans manually as a caller and that way gaining control over their lifecycle. |
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 However what we might do is define the destruction in relation to the invoker's target method. So something like:
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. |
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 We might require an explicit signal for "don't destroy For example, here are the transformers one would need to write and register for a 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 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:
I think I'm just going to remove the last commit that I added and pretend that this discussion has never happened 😆 |
0b8a36f
to
a52c27b
Compare
Rebased, squashed the 2nd commit (parameters of target methods are not injection points), removed the 3rd commit (deferred destruction of |
Added one commit to address this:
To be squashed. |
5a485bf
to
9c41e87
Compare
Also, here's a proposal for a "last resort" kind of resolution for the problem with destroying
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one commit to address this:
Now, I think the wording "the
invoke()
method of the built invoker shall ignore the given element of thearguments
array" can possibly be interpreted as a relaxation of the aforementioned rule, so perhaps it's something we should clarify.
The new wording is pretty clear and explicit; I like it!
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. |
I was also thinking about something like this:
We could add something like But I'm not exactly sure if that's helpful. It feels overspecified and underspecified at the same time 🤷 |
I think that wording makes sense, though I would change it to:
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
I agree, this doesn't seem helpful. You'd want the API to be Having another think about a possible spec API, would specifying the /**
* 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 |
Squashed and merged into |
I think your 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:
So I'm thinking something like this could work:
|
No description provided.