On Tue, 25 Apr 2023 21:16:41 GMT, Sergey Chernyshev <d...@openjdk.org> wrote:
> Hi all, > > I would like to propose a patch for an issue discussed in [1][2] that fixes > an OOME in the current code base. The issue appears when a SunJCE provider > object reference is stored in a non-static variable, which eventually leads > to a Java heap OOME. > > The solution proposed earlier [1] raised a concern that the identity of > provider objects may be somehow broken when using `WeakHashMap<Class<? > extends Provider>, Object>` instead of `IdentityHashMap<Provider, Object>`. > The solution hasn't been integrated that time. Later in 2019 a performance > improvement was introduced with [3][4] that changed `IdentityHashMap` to > `ConcurrentHashMap` of `IdentityWrapper<Provider> `objects that maintained > the object identity while improved performance. > > The solution being proposed keeps up with performance improvement in [3], > further narrowing the synchronization down to the hash table node, avoids > lambdas that may cause startup time regressions and maintains providers > identity using `WeakIdentityWrapper` that extends `WeakReference<Object>` > while avoiding depletion of Java heap by using weak pointers as the mapping > key. Once a provider object becomes weakly reachable it is queued along with > its reference object to the `ReferenceQueue` (a new static member in > `JceSecurity`). The `equals()` method of the `WeakIdentityWrapper` will never > match a new wrapper object to anything inside the hash table after the > corresponding element's `WeakReference.get()` returned null. This leads to > accumulating "empty" elements in the hash table. The new static function > `expungeStaleWrappers()` drops the "empty" elements queued by GC each time > the `getVerificationResult()` method is called. > > `ConcurrentHashMap`'s `computeIfAbsent()` does (partially) detect recursive > updates for keys that are being added to empty bins. For such keys an > `IllegalStateException` is thrown prior to recursive execution of the > `mappingFunction`. For nodes that are added to existing TreeBins or linked > lists, in which case no recursion detection is done prior to calling > `mappingFunction`, the recursion detection relies on old method with > `IdentityMap` of Providers. > > I include a test that runs under memory constrained conditions (128M) that > hits the heap limit in current VMs where it is impossible to reclaim > providers' memory (unless they've been cached under weak references). The > updated jdk versions pass the test. > > Testing: jtreg + JCK on a downport of the patch to JDK 17 with no regressions > > [1] https://mail.openjdk.org/pipermail/security-dev/2016-October/015024.html > [2] https://bugs.openjdk.org/browse/JDK-8168469 > [3] https://bugs.openjdk.org/browse/JDK-7107615 > [4] > https://github.com/openjdk/jdk/commit/74d45e481d1ad6aa5d7c6315ea86681e1a978ce0 @valeriepeng Could you please take a look at this change? ------------- PR Comment: https://git.openjdk.org/jdk/pull/13658#issuecomment-1534961823
