NIFI-15388: Leak in ReflectionUtils annotation cache.

[bobpaulin]

• WeakHashMap does not work properly to remove keys and values due to
  strong reference to the Key found in the Cache Value
• Keys should be programatically invalidated when the ClassLoader is
  closed to allow classes to be Garbage Collected.
• Moved ReflectionUtils to nifi-nar-utils so it can be shared by
  nifi-framework-nar-utils and nifi-framework-components

Summary

NIFI-15388

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

• Apache NiFi Jira issue created

Pull Request Tracking

• Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
• Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

• Pull Request based on current revision of the main branch
• Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Add DBCPConnectionPool Controller Service
Enable with PostgresJDBC driver properties
Disable Controller Service
Remove Controller Service

Expected result

Classes should be GC'ed when InstanceClassLoader is GCed.

Actual

Classes still have strong references with the methods stored in the ReflectionUtils annotationCache.

With code change Classes can be unloaded because values are stored as WeakReferences and can be GC.

https://localhost:8443/nifi-api/system-diagnostics/jmx-metrics?beanNameFilter=ClassLoading

{
    "jmxMetricsResults": [
        {
            "beanName": "java.lang:type=ClassLoading",
            "attributeName": "Verbose",
            "attributeValue": false
        },
        {
            "beanName": "java.lang:type=ClassLoading",
            "attributeName": "TotalLoadedClassCount",
            "attributeValue": 117051
        },
        {
            "beanName": "java.lang:type=ClassLoading",
            "attributeName": "UnloadedClassCount",
            "attributeValue": 31115
        },
        {
            "beanName": "java.lang:type=ClassLoading",
            "attributeName": "LoadedClassCount",
            "attributeValue": 85936
        },
        {
            "beanName": "java.lang:type=ClassLoading",
            "attributeName": "ObjectName",
            "attributeValue": {
                "domainPattern": false,
                "propertyListPattern": false,
                "propertyValuePattern": false,
                "propertyPattern": false,
                "canonicalKeyPropertyListString": "type=ClassLoading",
                "keyPropertyList": {
                    "type": "ClassLoading"
                },
                "keyPropertyListString": "type=ClassLoading",
                "domain": "java.lang",
                "pattern": false,
                "canonicalName": "java.lang:type=ClassLoading"
            }
        }
    ]
}

Build

• Build completed using ./mvnw clean install -P contrib-check
  • JDK 21
  • JDK 25

Licensing

• New dependencies are compatible with the Apache License 2.0 according to the License Policy
• New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

• Documentation formatting appears as expected in rendered files

[exceptionfactory]

Thanks for proposing this change @bobpaulin. The adjustment appears to be logical, but some sporadic test failures raise questions about unintended consequences. This might be an opportunity to consider switching some references to Apache Commons Lang3 MethodUtils, depending on whether certain uses should participate in the caching of Class and Method references. The framework uses this class in a number of places, so if there are only a few places where Method references should not be cached, it may be cleaner to switch to MethodUtils, instead of making this more fundamental change.

[bobpaulin]

Hi @exceptionfactory you are correct that I could replace ReflectionUtils with MethodUtils to invoke annotated methods without caching in strategic places that do not benefit from caching. I also agree that making that change in a few strategic locations could reduce the risk given how widely used this class is. I'm open to going in that direction in the short term. That said I'd like to lean in a bit to on these failures. There is a fundamental issue with using a WeakHashMap when the values (specifically the methods) contain references to the keys (the class). It effectively makes the WeakHashMap contain strong references which prevents it from serving it's intended purpose here.

I've just rebased the code on the latest main and will also see if I can reproduce these failures locally to determine if there is in fact a logical problem or if perhaps there's something else going on.

Thanks for proposing this change @bobpaulin. The adjustment appears to be logical, but some sporadic test failures raise questions about unintended consequences. This might be an opportunity to consider switching some references to Apache Commons Lang3 MethodUtils, depending on whether certain uses should participate in the caching of Class and Method references. The framework uses this class in a number of places, so if there are only a few places where Method references should not be cached, it may be cleaner to switch to MethodUtils, instead of making this more fundamental change.

[exceptionfactory]

That said I'd like to lean in a bit to on these failures. There is a fundamental issue with using a WeakHashMap when the values (specifically the methods) contain references to the keys (the class). It effectively makes the WeakHashMap contain strong references which prevents it from serving it's intended purpose here.

Thanks @bobpaulin, that's a very good point. It does seem worth tracing this down a bit more in light of the failures surfaced.

[bobpaulin]

Looks like the reason these test may be failing is due to the Method objects created from the Class are copies instead of directly referenced objects from Class object. Because of this the weak References to the methods will be eligible for garbage collection. This may be the reason the system test were failing. I experimented with a few different approaches including switching to MethodHandles which also leaves you with a reference to the class object which will prevent the WeakHashMap from doing what it is suppose to. So I've got a different approach that removes the keys when the instance classloader is closed. This also provides the desired effect of allowing the classes to be garbage collected. Will update this PR for this approach and will change the title of this PR to better represent this change.