8000 Add default hashCode implementation to RetryConfiguration.AnnotationClassOrMethodPointcut by NorsaG · Pull Request #472 · spring-projects/spring-retry · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add default hashCode implementation to RetryConfiguration.AnnotationClassOrMethodPointcut #472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 23, 2024
Merged

Add default hashCode implementation to RetryConfiguration.AnnotationClassOrMethodPointcut #472

merged 1 commit into from
Sep 23, 2024

Conversation

NorsaG
Copy link
Contributor
@NorsaG NorsaG commented Sep 23, 2024 •
edited
Loading

This pull request introduces a default implementation of hashCode for RetryConfiguration.AnnotationClassOrMethodPointcut.

I discovered that using Spring Cloud with configuration refreshing (specifically the @RefreshScope annotation and ConfigServicePropertySourceLocator as part of spring-cloud-config) can lead to a "java.lang.OutOfMemoryError: Metaspace". This occurs because the ConfigServicePropertySourceLocator is re-initialized with every refresh. Due to the absence of a hashCode implementation, proxies for this class are regenerated each time, which causes them to be stored in the org.springframework.cglib.core.internal.LoadingCache (with RetryConfiguration.AnnotationClassOrMethodPointcut being part of the composite key for this object).
This results in an excessive accumulation of generated proxies in memory, ultimately leading to metaspace overflow.
8000

@pivotal-cla
Copy link

@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.

@pivotal-cla
Copy link

@NorsaG Thank you for signing the Contributor License Agreement!

@artembilan artembilan added this to the 2.0.10 milestone Sep 23, 2024
artembilan
artembilan requested changes Sep 23, 2024
View reviewed changes
Copy link
Member
@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update Copyright of the changed class to actual year.
Add you name to the @author list.

Thanks

@NorsaG NorsaG force-pushed the AnnotationClassOrMethodPointcut-hashCode-implementation branch from dfc2ee9 to 492e03d Compare September 23, 2024 17:15
@NorsaG NorsaG force-pushed the AnnotationClassOrMethodPointcut-hashCode-implementation branch from 80d4edf to b4c9058 Compare September 23, 2024 17:16
@NorsaG
Copy link
Contributor Author
NorsaG commented Sep 23, 2024

Fit the PR according to the comment.

@artembilan artembilan merged commit 76b41e8 into spring-projects:main Sep 23, 2024
2 checks passed
@artembilan
Copy link
Member

@NorsaG ,

thank you for contribution; looking forward for more!

@NorsaG
Copy link
Contributor Author
NorsaG commented May 21, 2025

@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

@artembilan
Copy link
Member

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!

@NorsaG
Copy link
Contributor Author
NorsaG commented May 23, 2025 •
edited
Loading

@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).
1

In Picture 2, nothing is found in the cache.
2

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

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

@artembilan
Copy link
Member

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();
		}
  1. 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.

@NorsaG
Copy link
Contributor Author
NorsaG commented May 23, 2025

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

@artembilan
Copy link
Member

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.

@NorsaG
Copy link
Contributor Author
NorsaG commented May 25, 2025

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();

@artembilan
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artembilan artembilan artembilan requested changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
2.0.10
Development

Successfully merging this pull request may close these issues.
3 participants
@NorsaG @pivotal-cla @artembilan
0