@NorsaG Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@NorsaG Thank you for signing the Contributor License Agreement!

Fit the PR according to the comment.

@NorsaG ,

thank you for contribution; looking forward for more!

@artembilan, I’ve finally published an article with a detailed explanation of a real-world case of this issue: https://shorturl.at/HVa0Y
I think it could be interesting

Hi @NorsaG !

The article is great!
Sorry to cause so much pain and great to see how you have figured out everything.

Would you mind from your experience point me where exactly that RetryConfiguration.AnnotationClassOrMethodPointcut is used a part of cache key.

Thank you!

@artembilan Yes, of course.

I've tried to reproduce the same problem again. It was a bit tricky due to the complexity of our project and the ongoing component upgrades.

In Picture 1, you can observe the LoadingCache object at the moment of retrieving an object from the cache (I paused at a debug point after several configuration updates from the config server).

In Picture 2, nothing is found in the cache.

In Picture 3, I have highlighted the path to the pointcut.

(I'm not entirely sure, but I highlighted the "methodMatcherKey" string because it is unique for all EnhancerKeys in the cache.)

Thanks. I see now.
So, the path is like this:

1. org.springframework.aop.framework.AdvisedSupport:

private List<Advisor> advisorKey = this.advisors;

Where one of them is our RetryConfiguration.
2. org.springframework.aop.framework.CglibAopProxy.ProxyCallbackFilter:

public int hashCode() {
			return this.advised.getAdvisorKey().hashCode();
		}

3. org.springframework.cglib.proxy.Enhancer.createHelper():

		Object key = new EnhancerKey((superclass != null ? superclass.getName() : null),
				(interfaces != null ? Arrays.asList(ReflectUtils.getNames(interfaces)) : null),
				(filter == ALL_ZERO ? null : new WeakCacheKey<>(filter)),
				Arrays.asList(callbackTypes),
				useFactory,
				interceptDuringConstruction,
				serialVersionUID);

So, apparently the problem has gone with that our hashCode() fix just because the RetryConfiguration uses a default implementation based on its properties.

I see how this was hard to catch and even harder to determine what simple fix has to be.

Thank you again!

I will look now to other Advisor implementations for similar problem.

That was a really interesting issue! It was a bit of a pain to track down the cause since the production service wasn’t accessible, but I’m happy I could help out

I was wrong with that sentence:

So, apparently the problem has gone with that our hashCode() fix just because the RetryConfiguration uses a default implementation based on its properties.

It is not about properties, but object identity by default: https://varoa.net/jvm/java/openjdk/biased-locking/2017/01/30/hashCode.html.

And you were right, the AdvisorKeyEntry is the root of the logic:

this.methodMatcherKey = pointcut.getMethodMatcher().toString();

That getMethodMatcher() is like this for our RetryConfiguration.AnnotationClassOrMethodPointcut:

	public final MethodMatcher getMethodMatcher() {
		return this;
	}

and that default toString() implementation is:

    public String toString() {
        return getClass().getName() + "@" + Integer.toHexString(hashCode());
    }

So, that's where we got that discrepancy because default hashCode() is an identity of the new object after refresh scope.

I read the article above, but in my opinion, the author described a some different set of issues.

According to the official API note:

It is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.

Source: Object.equals() documentation

This was a significant problem in our case.

Another point is the toString() method. While debugging the latest version of the code, I noticed it, specifically when I saw the default toString() output for methodMatcherKey.
It might be a good idea to override this method, especially considering its usage here:
this.methodMatcherKey = pointcut.getMethodMatcher().toString();

I think toString() would be that unnecessary extra.
The main point that we have missed to implement properly equals() and hashCode().
Nothing more to worry about!