On Wed, 28 Oct 2020 18:01:44 GMT, Rémi Forax <github.com+828220+fo...@openjdk.org> wrote:
>>> 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) Hi Remi, I appreciate your proposal to modernize Proxy API. There are several requests for this enhancement to support default methods in Proxy. Defining a new `java.lang.invoke.Proxy` is a much bigger project that I can't tell when the existing users of `java.lang.reflect.Proxy` will be able to get this default method invocation support. I do agree that this API design has many challenges caused by what you listed above. Well, I believe we are very close to have a consensus: 1. New `newProxyInstance` factory method takes a handler factory doing the access check 2. Update `getInvocationHandler` to throw `InaccessibleInvocationHandlerException` if access denied to get an invocation handler associated with the proxy instance If this needs more time, I think we can consider to shelf this RFE and come back to it later (and consider your proposal as well). ------------- PR: https://git.openjdk.java.net/jdk/pull/313
