RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now"

[Attila Szegedi]

On Fri, 19 Feb 2021 11:22:57 GMT, Peter Levart <plevart at openjdk.org> wrote:

>I would like 1st to understand why the MH created in the TestLinker.convertToType is actually GCed at all.

The whole original issue was about allowing these objects to be GCd ��.
Short story: because all data is scoped to objects created within `makeOne` method.

Longer story: It is GC'd because its reachability is dominated by the `DynamicLinker` object created on [line 96](https://github.com/openjdk/jdk/blob/3afec32bdf49703bb3537b36248f0550caae6d87/test/jdk/jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java#L96). It and everything it dominates (`LinkerServicesImpl` object holding the `TypeConverterFactory` object holding the `BiClassValue` objects holding the method handles) become garbage as soon as the method exits. This works because of the pains I took in `BiClassValue` to ensure we don't leak any of the infrastructural objects through implicit `this$0` outer-class references into the `ClassValue`s. That allows the `ClassValue` objects to be GCd and removed from the classes' class-value mapping. This ordinarily doesn't happen as most `ClassValue` objects are bound to static fields, but in `TypeConverterFactory` the `BiClassValue` instances are specific to the `TypeConverterFactory` instance, so they are expected to be reclaimable by G
 C if the converter factory itself goes away.

 > And after that, why it is not GCed before 12 invocations to makeOne() are made.

Because GC is at liberty to delay recognizing an object is phantom reachable? I don't think we need to read too much into this? Correct me if I don't see something. 

> What would be more interesting to test is to create a converter between a type loaded by a custom (child) ClassLoader and a system type. After that, release all hard references to the custom ClassLoader and see if the converter gets GC-ed as a result. WDYT?

That does sound like [TypeConverterFactoryRetentionTests.testSystemLoaderToOtherLoader](https://github.com/openjdk/jdk/blob/3afec32bdf49703bb3537b36248f0550caae6d87/test/jdk/jdk/dynalink/TypeConverterFactoryRetentionTests.java#L118) True, it tests whether the _class loader_ itself gets released, not the converter, but the loader would hardly get released while there's a converter with the class from that loader bound in the parameter signature of its method handle, wouldn't it?

Regardless, if you think there's a valid use case for this additional test, we can discuss it. I'd rather scope this issue to fixing the flakiness of the tests, so they don't cause trouble with builds and can be backported/added to ongoing backports.

Thanks for taking time to look into this!

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

PR: https://git.openjdk.java.net/jdk/pull/2617