search

spring-projects / spring-retry

2.15k stars 514 forks source link

Support concurrent behavior on `MetricsRetryListener` #467

Closed huisam closed 1 week ago

huisam commented 1 week ago

Hello Spring team. I was glad to hear that official support MetricsRetryListener class by this issue https://github.com/spring-projects/spring-retry/issues/458

But I have a question about this class behavior.

Question

The retryContextToSample field is considered for reusable many retry operations. And the RetryListener can be registered by bean from RetryConfiguration class.

If the MetricsRetryListener registered by bean, the MetricsRetryListener can called by concurrent retry operations.

In conclusion, my question is why retryContextToSample was defined to IdentityHashMap ? 

private final Map<RetryContext, Timer.Sample> retryContextToSample = new IdentityHashMap<>();

On IdentityHashMap javadoc, it doesn't support thread safe behavior.

Suggestion

Can you support the thread safe behavior from the retryContextToSample?

private final Map<RetryContext, Timer.Sample> retryContextToSample = new ConcurrentHashMap<>();

Discussion

Please feel free to comment on this issue. If my suggestion is wrong, then I will close this issue.

artembilan commented 1 week ago

See discussion in the PR how we have introduced an IdentityHashMap: https://github.com/spring-projects/spring-retry/pull/459#discussion_r1699187723. Well, since it complains that put/remove are not thread-safe, than I'll wrap it into a Lock.

Thank you for pointing that out!

artembilan commented 1 week ago

After thinking one more time, the Collections.synchronizedMap() wrapping would be enough: we just don't do any IO operations over there to worry about platform thread pinning.

Feel free to contribute the fix!

huisam commented 1 week ago

I see. I understood about the IdentityHashMap background. I think the same way you think. It will be enough to wrapping the IdentityHashMap by Collections.synchronizedMap for supporting concurrent behavior.

Then I will submit the pull request, please review the pull request for next release!

basovnik commented 6 days ago

@artembilan I am facing the issue that should be fixed by this commit. When are you going to release it?

j.l.IllegalStateException: No 'Timer.Sample' registered for '[RetryContext: count=0, lastException=null, exhausted=false]'. Was the 'open()' called?
    at o.s.util.Assert.state(Assert.java:97)
    at o.s.r.s.MetricsRetryListener.close(MetricsRetryListener.java:113)
    at o.s.r.s.RetryTemplate.doCloseInterceptors(RetryTemplate.java:619)