NIFI-15388: Values in WeakHashMap should be wrapped by WeakReference.

[bobpaulin]

• Wrapping Values with WeakReference will allow Classes to be GCed.

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.