RFR: 8159746: (proxy) Support for default methods

[Rémi Forax]

On Wed, 28 Oct 2020 17:36:40 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>>> Hi Peter,
>>> 
>>> > Question remains how do we distinguish proxies with old-fassioned InvocationHandlers from proxies with InvocationHandlers having access to super-default-handler. Both kinds of handlers look the same from Proxy's perspective... Perhaps we need a boolean flag in Proxy instance for that, coupled with an overloaded constructor that takes it...
>>> 
>>> I have the Proxy constructor taking both the invocation handler and `superHandler` in my [mlchung:proxy-default-method-4](https://github.com/mlchung/jdk/tree/proxy-default-method-4) branch.
>>> 
>> 
>> Yes, but it is not really useful there. The super handler is captured by the user invocation handler created in the invocation handler factory function and it is only used by the user invocation handler. All we need in the proxy instance is a boolean flag indicating the way the invocation handler was created (which one of the two overloaded newProxyInstance methods was called).
>> 
>>> > Problem with this approach is that it is not really useful in the proxy instance. We don't need it in the proxy instance.
>>> 
>>> See above.
>>> 
>>> About the exception thrown: we need to distinguish CCE and NPE thrown by the default method vs thrown by arguments incompatible with the method signature. In my proxy-default-method-4 branch, I define an internal exception type for that. There is an overhead doing the wrapping but it's an exception case which performance should not be critical.
>> 
>> To be precise, we need to pass CCE and NPE thrown by the default method unchanged, but we need to wrap CCE and NPE thrown by the method handle transformation chain with IllegalArgumentException. Is that what you are referring to? In that case we need an internal exception type, not exposed to public API, used just to "tunnel" the exception from the default method unchanged. That's just implementation detail.
>> 
>> But we have to ask ourselves a question: "Do we want to wrap CCE and NPE thrown by the method handle transformation chain with IllegalArgumentException?". We did that in previous variants to mimic the Method.invoke specification. But now we already departed from that specification by not wrapping exceptions thrown by the default method with InvocationTargetException. So we are not obligated to follow the rest of the Method.invoke specification either. All exception types thrown by method handle transformation chain (CCE, NPE, IAE) are unchecked exceptions and as such are not eligible to wrapping with UndeclaredThrowableException by the Proxy API. So it's just a matter of deciding what is more suitable: to have more specific exception types (CCE, NPE, IAE) for describing transformation exceptions or to reduce them to one common type (IAE). In either case those exception types can also be thrown by the default method and we can't or even want to distinguish their source. Let's be ho
 nest, all of CCE, NPE, IAE are usually describing the subtle variants of the same thing: that the parameters are wrong. Whether this happens in the logic of invocation handler, method handle transformation chain or the default method, we want those exceptions to propagate out of the invoked proxy method unchanged.
>> 
>> So WDYT?
>
>> Yes, but it is not really useful there. The super handler is captured by the user invocation handler created in the invocation handler factory function and it is only used by the user invocation handler. All we need in the proxy instance is a boolean flag indicating the way the invocation handler was created (which one of the two overloaded newProxyInstance methods was called).
> 
> Either way provides the distinction if this proxy class supports default method invocation.   I chose to store the superHandler rather than a boolean that may allow us to do white-box testing (an alternative to the trick in getting the superHandler in spinning another proxy instance).   It's a trivial change  if we prefer to store a boolean field but keep the constructor to take `superHandler` parameter.  I'm fine with it.
> 
>> To be precise, we need to pass CCE and NPE thrown by the default method unchanged, but we need to wrap CCE and NPE thrown by the method handle transformation chain with IllegalArgumentException. Is that what you are referring to? 
> 
> Yes and it's implementation details.  For the specification side, I agree that `InvocationHandler::invoke` throws the exception by the method and no need to wrap it with `InvocationTargetException` .
> 
>> "Do we want to wrap CCE and NPE thrown by the method handle transformation chain with IllegalArgumentException?".
> 
> I did consider that and propose to go with IAE for simplicity and consistency with the core reflection (`Method::invoke`, `Field::set` etc) when there is an illegal argument.   Is it worth specifying `CCE` and  `NPE` explicitly when an argument fails the conversion?   Maybe but average users of `java.lang.reflect` may not use `java.lang.invoke` and `IAE` is thrown for illegal argument to the method invocation.   When invoking a default method, `Method` object representing a default method is passed to `superHandler` and it's intuitive to consider the behavior as if `Method::invoke` is called (note that the specification does not mandate/expect to use `Lookup::findSpecial` and `MethodHandle::invoke*`).   While CCE and NPE provide slightly clearer reason of the illegal argument, as you said, they are subtle variants of the same thing indicating the parameters are wrong.  This only impacts the performance of the exception cases which should not be a big concern.    This explains why I
  am in favor to throw IAE for illegal arguments which is the expected behavior for average core reflection users.  
> 
>> What to do about that. Is UncheckedReflectiveOperationException a suitable solution? 
> 
> I do see your point of proposing `UncheckedReflectiveOperationException` which I thought about too and was tempted.  I'm uncertain yet if we want a  unchecked  exception type for any reflective operation exceptions.     Should future new `java.lang.reflect` and `java.lang.invoke` APIs use the unchecked exception?  This is a broader question to answer than one specific exception type `java.lang.reflect.InaccessibleInvocationHandlerException`??
> 
>> I'm not particularly fond of creating another Proxy.invocationHandler method, because it does not solve the problem of the old method that may also be called.
> 
> Me neither.

Hi Mandy and Peter,
reading your exchanges, i wonder if it's not better to stop trying to add more and more features to the already clunky java.lang.reflect.Proxy but instead to move to provide the same set of feature but using an implementation based on java.lang.invoke.Lookup/MethodHandle.

I propose to introduce a java.lang.invoke.Proxy has a replacement of java.lang.reflect.Proxy using j.l.i.MethodHandle instead of j.l.r.Method, the equivalent of the InvocationHandler acting as a linker called once per the first time a method of the Proxy is called instead of being called each time a method is called.

Such proxy
- has it's security model based on Lookup to define the class,
  so no supplementary restrictions on package/module of the interface implemented
- avoid to mix java.lang.reflect API and java.invoke API
- is transparent in term of exceptions (ther are propagated without any wrapping/unwrapping)
- can support default methods natively
- doesn't requires any caching (the user will be responsible to do the caching)
- is more efficient, actually as efficient as the equivalent written code or a little more (thanks to @ForceInline)

-------------

PR: https://git.openjdk.java.net/jdk/pull/313