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] http... This pull request has now been integrated. Changeset: a284920b Author: Sergey Chernyshev <serge.chernys...@bell-sw.com> Committer: Valerie Peng <valer...@openjdk.org> URL: https://git.openjdk.org/jdk/commit/a284920b3432b00496a2a32a284a91a9bd49fb06 Stats: 111 lines in 2 files changed: 84 ins; 8 del; 19 mod 8168469: Memory leak in JceSecurity Reviewed-by: valeriep ------------- PR: https://git.openjdk.org/jdk/pull/13658
