RFR: 8159746: (proxy) Support for default methods

[Peter Levart]

On Thu, 15 Oct 2020 18:30:20 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> Hi Mandy,
>> 
>> I'm beginning to think that we somewhat wondered away from the original intent which was to mimic the bytecode
>> behaviour as much as possible. Bytecode behaviour for super calls (invokespecial) is very special and what you do in
>> the last patch is guarding invocation to a super default method via @CallerSensitive method which checks the access of
>> the caller as though this was a normal call. Similar access check when a default method might reside in a
>> package-private class or when this class might not be exported is already performed when the code creates a Proxy
>> class/instance (Proxy::getProxyClass/newProxyInstance). This code also designates an InvocationHandler for the proxy
>> instance. From that point on, the designated invocation handler should have full access to invoking super default
>> methods on the proxy instance regardless of where it resides because it has been chosen as the delegate of the Proxy
>> class with full powers.  Since we are exposing the invokeDefaultMethod functionality in a public static method, it is
>> hard (or not quite possible) to check that the code doing the invocation is in fact the designated invocation handler
>> code. One approximation might be the following check:  `Proxy.getInvocationHandler(proxy).getClass() == callerClass`
>> ...but that would not work for code residing in the invocation handler superclass even though such code might be
>> eligible to call the super default method. OTOH such check is over permissive as it allows calling the super method
>> from static context too.  So I was thinking about an alternative approach. To expose the method as a protected final
>> instance method in an abstract class implementing InvocationHandler:  public interface InvocationHandler {
>>   ...
>>   abstract class Support implements InvocationHandler {
>>     protected final Object invokeDefaultMethod(Object proxy, Method method, Object... args) throws
>>     InvocationTargetException {
>>       if (this != Proxy.getInvocationHandler(proxy)) throw ...
>>       ...
>>     }
>>   }
>> }
>> 
>> Users would have to extend `InvocationHandler.Support` to access this feature. This might be OK for most users, but
>> those wanting to extend some other class will not be able to do that.
>> So WDYT? In case you still prefer your approach of re-checking the permissions in invokeDefaultMethod, then I might
>> have some ideas of how to speed-up theses checks. As currently stands, invoking public exported interface default
>> methods does exhibit a little more penalty, but for non-public or non-exported interfaces, the penalty is bigger:
>> Mandy's original:
>> Benchmark             Mode  Cnt    Score    Error  Units
>> ProxyBench.implClass  avgt    5    3.709 ±  0.026  ns/op
>> ProxyBench.implProxy  avgt    5  926.215 ± 11.835  ns/op
>> 
>> Peter's performance patch:
>> 
>> Benchmark             Mode  Cnt   Score   Error  Units
>> ProxyBench.implClass  avgt    5   3.777 ± 0.005  ns/op
>> ProxyBench.implProxy  avgt    5  27.579 ± 0.250  ns/op
>> 
>> Moved to InvocationHandler + added access check
>> 
>> Benchmark               Mode  Cnt    Score   Error  Units
>> ProxyBench.implClass    avgt    5    3.740 ± 0.004  ns/op
>> ProxyBench.implProxy    avgt    5   34.226 ± 0.125  ns/op
>> ProxyBench.ppImplClass  avgt    5    3.780 ± 0.004  ns/op
>> ProxyBench.ppImplProxy  avgt    5  147.318 ± 1.422  ns/op
>> 
>> The updated benchmark code is here: https://gist.github.com/plevart/f6e0a5224c3ffbf14b28b47755599226
>> (the ppImpl... benchmarks are against package-private interface)
>
> Hi Peter,
> 
> The current `newProxyInstance` API does not perform access check against the proxy interfaces.    I want to avoid
> adding new caller-sensitive method where possible.   I did consider any alternative to make it non-static method which
> only allows a proxy's invocation handler to call `invokeDefaultMethod` if `proxy.getInvocationHandler() == this`.  If
> access check done at invocation time, `invokeDefaultMethod` still needs to be caller-sensitive which must be a final
> method (can't be on a default method)  In addition, an invocation handler to call `invokeDefaultMethod` can't be
> implemented in a lambda because it can't self-reference itself.  So the invocation handler that wants to invoke default
> methods must be a concrete or anonymous class which is a limitation.    Alan and I also discussed other alternative
> earlier.   I agree to prototype a slightly different alternative that will perform access check at proxy instance
> creation time: 1. define an abstract `DelegatingInvocationHandler` class that defines a protected final `invokeDefault`
> method (shortened method name). 2. `Proxy::newProxyClass` will perform access check if the given `InvocationHandler` is
> an instance of `DelegatingInvocationHandler` 3. `Proxy(InvocationHandler h)` constructor should throw an exception if
> the given `h` is an instance of `DelegatingInvocationHandler`.  `Proxy::getProxyClass` is deprecated and it's
> recommended to call `newProxyInstance` instead.  It's strongly discouraged to invoke the Proxy constructor and so I
> think not supporting `DelegatingInvocationHandler` may be reasonable. 4. `Proxy::getInvocationHandler` needs to check
> if the caller class has access to the proxy interfaces if it's a `DelegatingInvocationHandler`.  So one can't get a
> proxy instance whose invocation handler can invoke default methods if the caller has no access to the proxy interfaces;
> similarly one can't get a `DelegatingInvocationHandler` from a proxy instance if the caller has no access.  This
> alternative avoids `invokeDefault` being caller-sensitive but the limitation of `DelegatingInvocationHandler`
> implementation in a concrete/anonymous class.  Thought/feedback?

Hi Mandy,

I read your concerns and have an alternative API proposition here. Instread of an abstract class with protected final
`invokeDefaultMethod` method, we can keep the public static `invokeDefaultMethod` and add a `Lookup` parameter to it.
The Lookup instance to be passed to the method should be able to `findSpecial` the default method - it has to be the
full privilege lookup with proxy class as lookup class. Such instance is provided in a call to the alternative
invocation handler: `InvocationHandlerWithLookup` which is a super-interface of plain `InvocationHandler` that just
forwards the invocation with the additional Lookup parameter to the abstract method without parameter (in order to keep
backwards-compatibility). The generated proxy class invokes this new method in new interface and passes its own lookup
to it. There's also an overloaded Proxy::newProxyInstance method that takes this new InvocationHandlerWithLookup
parameter instead of plain InvocationHandler. This allows specifying lambdas for both overloaded methods and the right
one is selected according to the number of lambda parameters. invoking the constructor of the proxy class taking plain
InvocationHandler is still allowed but only if the handler class does not use the Lookup parameter. The overloaded
Proxy.newProxyInstance throws IllegalAccessException in case the passed-in handler uses the provided lookup but some
interfaces of the proxy are not accessible.

Performance with this implementation is restored and is not worse when dealing with inaccessible interfaces:

Mandy's original:

Benchmark             Mode  Cnt    Score    Error  Units
ProxyBench.implClass  avgt    5    3.709 ±  0.026  ns/op
ProxyBench.implProxy  avgt    5  926.215 ± 11.835  ns/op


Peter's performance patch:

Benchmark             Mode  Cnt   Score   Error  Units
ProxyBench.implClass  avgt    5   3.777 ± 0.005  ns/op
ProxyBench.implProxy  avgt    5  27.579 ± 0.250  ns/op


Moved to InvocationHandler + added access check

Benchmark               Mode  Cnt    Score   Error  Units
ProxyBench.implClass    avgt    5    3.740 ± 0.004  ns/op
ProxyBench.implProxy    avgt    5   34.226 ± 0.125  ns/op
ProxyBench.ppImplClass  avgt    5    3.780 ± 0.004  ns/op
ProxyBench.ppImplProxy  avgt    5  147.318 ± 1.422  ns/op


Alternative API with access check in newProxyInstance

Benchmark               Mode  Cnt   Score   Error  Units
ProxyBench.implClass    avgt    5   3.782 ± 0.013  ns/op
ProxyBench.implProxy    avgt    5  32.493 ± 0.192  ns/op
ProxyBench.ppImplClass  avgt    5   3.749 ± 0.002  ns/op
ProxyBench.ppImplProxy  avgt    5  30.565 ± 0.190  ns/op

The code is in this pull request against your last commit in your branch: https://github.com/mlchung/jdk/pull/2

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

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