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.

> > _the unsynchronised access to the data structures doesn't gain anything_
> 
> The existing optimistic fast path can avoid the lock, with the assumption that garbage collection of the cache values are rare. It is fine. However these plain access do require that the cached values to be thread safe and support safe publication.

You're right, the lock in `removeStaleEntries` is acquired if there's a reference to remove.

Accessing the `table` field isn't safe without synchronisation. When the table is re-allocated, it may go awry.

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

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