RFR: 8344892: beans/finder/MethodFinder.findMethod incorrectly returns null

[Alexey Ivanov]

On Fri, 28 Feb 2025 14:48:45 GMT, Alexander Zvegintsev <azvegint at openjdk.org> wrote:

>> During the [JDK-8344891 Remove uses of sun.misc.ReflectUtil in java.desktop](https://bugs.openjdk.org/browse/JDK-8344891) it was discovered that `beans/finder/MethodFinder.findMethod' incorrectly returned null if a signature was not in the cache and added it to the cache if it was already there:
>> 
>> 
>> Method method = CACHE.get(signature);
>> return (method == null) ? method : CACHE.create(signature);
>> 
>> This resulted in a [significant drop in performance](https://bugs.openjdk.org/browse/JDK-8350573).
>> 
>> ----
>> 
>> Before ReflectUtil was removed, it worked by coincidence:
>> 
>> 
>> Method method = CACHE.get(signature);
>> return (method == null) || isPackageAccessible(method.getDeclaringClass()) ? method : CACHE.create(signature);
>> 
>> 
>> 
>> 
>> 1. `Cache#get` non-obviously creates a value in Cache, this in turn allowed us to avoid the NPE in the `(method == null) || isPackageAccessible(method.getDeclaringClass())` condition
>> 
>> 
>> https://github.com/openjdk/jdk/blob/d6c4be672f6348f8ed985416ed90d0447f5d5bb3/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L120-L126
>> 
>> 2. `isPackageAccessible(method.getDeclaringClass())` was evaluated as true
>> 
>> This is how we previously returned the cached value.
>> 
>> ---
>> 
>> So the solution is obvious:
>> 
>> 
>> Method method = CACHE.get(signature);
>> return (method != null) ? method : CACHE.create(signature);
>> 
>> 
>> Testing is green.
>
> src/java.desktop/share/classes/com/sun/beans/finder/MethodFinder.java line 81:
> 
>> 79:         try {
>> 80:             Method method = CACHE.get(signature);
>> 81:             return (method != null) ? method : CACHE.create(signature);
> 
> This can be simplified to the `return CACHE.get(signature)`, since we know that the implementation creates the value in the get method:
> 
> https://github.com/openjdk/jdk/blob/d6c4be672f6348f8ed985416ed90d0447f5d5bb3/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L120-L126
> 
> However, I am not a fan of this solution as it is implementation dependent and this behavior is not documented.

I think this is confusing… If `CACHE.get(signature)` always returns a non-null value, there's no need for the condition `method != null` — it's always true.

The javadoc for the `Cache` class specifies a `null` value can be returned:

https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L98-L99

Is it a bug in `Cache` class that it creates the value and adds it to the cache when the value isn't found?

https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L126-L127

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23845#discussion_r1975832067