RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v3]
Peter Levart
plevart at openjdk.java.net
Fri Sep 3 14:23:28 UTC 2021
On Thu, 2 Sep 2021 11:35:52 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:
>> `MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle` and it can introduce a class loader leak through its `MethodType`.
>>
>> Proposed fix introduces a 2-level cache (1 element each) where 1st level can only contain `MethodHandle`s which are guaranteed to not introduce any dependencies on new class loaders compared to the original `MethodHandle`. 2nd level is backed by a `SoftReference` and is used as a backup when the result of `MethodHandle.asType()` conversion can't populate the higher level cache.
>>
>> The fix is based on [the work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) made by Peter Levart @plevart back in 2015.
>>
>> Testing: tier1 - tier6
>
> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>
> Address review comments
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 877:
> 875: }
> 876: if (asTypeSoftCache != null) {
> 877: atc = asTypeSoftCache.get();
NPE is possible here too! asTypeSoftCache is a non-volatile field which is read twice. First time in the if (...) condition, 2nd time in the line that de-references it to call .get(). This is a data-race since concurrent thread may be setting this field from null to non-null. Those two reads may get reordered. 1st read may return non-null while 2nd may return null. This can be avoided if the field is read just once by introducing a local variable to store its value.
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 878:
> 876: if (asTypeSoftCache != null) {
> 877: atc = asTypeSoftCache.get();
> 878: if (newType == atc.type) {
NPE is possible here! act can be null as it is a result of SoftReference::get
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 933:
> 931: }
> 932:
> 933: /* Returns true when {@code loader} keeps {@code mt} either directly or indirectly through the loader delegation chain. */
Well, to be precise, loader can't keep mt alive. It would be better to say "keeps mt components alive" ...
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 948:
> 946: if (isBuiltinLoader(defLoader)) {
> 947: return true; // built-in loaders are always reachable
> 948: }
No need for special case here. isAncestorLoaderOf(defLoader, loader) already handles this case.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5246
More information about the core-libs-dev
mailing list