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

[Chen Liang]

On Fri, 28 Feb 2025 18:06:28 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> 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

Sorry for my last premature comment.

I have looked at `Cache`, and believe `create` should be made protected to indicate it is not intended to be called, but only overridden.

The preexisting code called `create` to reuse the creation mechanism without going through the cache - such usage is dubious. Per my experience, similar conditional caches are better implemented by enclosing the logic into the cache structure and use a common endpoint to access, so we have one universal site to determine the conditionality of caching. In this case, such a condition is better included in the `Cache` class itself, and the `create` method should not be publicly exposed.

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

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