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

[Alexey Ivanov]

On Fri, 28 Feb 2025 14:41:41 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.

It's somewhat off-topic, yet I find it *worrying*.

The `Cache.get` method starts with *unsynchronised access first*.

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

It doesn't make sense… First of all, if the method can be accessed concurrently, which seems to be implied, the `table` field could be in an inconsistent state. This could result in hard-to-reproduce bugs.

Secondly, the `Cache.get` invokes `removeStaleEntries` which has a `synchronized` block. That is the `get` method still requires explicit synchronisation. Having this in mind, *the unsynchronised access to the data structures doesn't gain anything*.

Performance-wise, it would be better to wrap the call to `removeStaleEntries` and the required logic into `synchronized (this.queue)`. 

Thirdly, there's another call to `removeStaleEntries` in the `get` method, this time it's inside the `synchronized (this.queue)` block.

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

PR Comment: https://git.openjdk.org/jdk/pull/23845#issuecomment-2691348842