Support concurrent behavior on MetricsRetryListener

[huisam]

Hello Spring team. I was glad to hear that official support MetricsRetryListener class by this issue #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]

See discussion in the PR how we have introduced an IdentityHashMap: #459 (comment).
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]

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]

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]

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