Avoid keeping a strong reference to caches being monitored

[slovdahl]

Fixes #2642.

[slovdahl]

I'm getting test failures locally in CaffeineCacheMetricsTest.reportExpectedGeneralMetrics, and I can't figure why that is. Works fine locally on upstream main.

[shakuzen]

The implementations of Gauge and FunctionCounter do not call the value function if the underlying obj is null. For example, in CacheMeterBinder, where it has

FunctionCounter.builder("cache.gets", cache.get(), c -> hitCount())

if the weak reference's get() returns null then the hitCount() method will not be called. This is why with the current implementation, we don't really care what the abstract methods return if the cache is GC'd because they won't be called. This behavior is not very apparent and took me a bit of looking at it to realize.

[slovdahl]

@shakuzen do you want me to fix all the caches in this same PR, or should I do it in separate PRs?

[slovdahl]

@shakuzen do you want me to add some tests for validating that no strong reference is kept? It's probably doable by doing one or more System.gc() calls after clearing the reference to the Cache instance.

[shakuzen]

Yes, I think the cause and fix is the same for all so fixing them all in the same pull request sounds reasonable to me. And yes, a test would be good. I have one locally I can work into the pull request if you don't feel like making one.

[slovdahl]

Yes, I think the cause and fix is the same for all so fixing them all in the same pull request sounds reasonable to me

I'll update the PR with the same kind of fix for all them then 👍

And yes, a test would be good. I have one locally I can work into the pull request if you don't feel like making one.

Sounds great!

[slovdahl]

Updated the PR accordingly. AFAICT, the JCacheMetrics class does not need any WeakReference wrapping. Do you agree with that?

[jonatan-ivanov]

This looks good to me the only thing I miss is testing this, I guess we have two issues here:

1. Forcing weak references to return null
2. Preventing weak references to return null (if GC will clean things up during tests, our current tests could break since we will get the default value)

@shakuzen Could you please share what do you have locally to test this?
I'm thinking about having another ctor that accepts a reference so that we can control it and simulate "GC" events (I'm not sure mocking (mockito) a reference can work at all):

public class MockReference<T> extends WeakReference<T> {
    private T referent;

    public MockReference(T referent) {
        super(referent);
        this.referent = referent;
    }

    @Nullable
    @Override
    public T get() {
        return this.referent;
    }

    public void simulateGC() {
        this.referent = null;
    }
}

[shakuzen]

@shakuzen Could you please share what do you have locally to test this?

It may depend on the JVM and GC, but at least in my testing locally, it was reliable enough to do the following to garbage collect the cache:

cache = null;
System.gc();

[shakuzen]

I added some generics and the dereference test to the cache metrics TCK. The current build failure looks unrelated. We'll see if we can get that resolved.

[shakuzen]

I was going to merge this to the maintenance branches originally, but with the addition of generics, it is more appropriate to include this in the next feature release. We can separately consider a back-port to maintenance branches without the generics if there is demand.