From f92c90a2a519c57e4ab74bcc3b0521cc9593f500 Mon Sep 17 00:00:00 2001
From: Artem Bilan <artem.bilan@broadcom.com>
Date: Fri, 22 Nov 2024 10:17:59 -0500
Subject: [PATCH] GH-477: Fix `MetricsRetryListener.close()` for concurrent
 calls

Fixes: https://github.com/spring-projects/spring-retry/issues/477

The `Timer.Builder` from Micrometer does not create a new `Builder` instance for its `tags()` call.
So, using shared `Timer.Builder` is not OK when it can be used from concurrent calls.

* Remove shared `retryMeterProvider` property and use fresh `Timer.Builder` instance in the `MetricsRetryListener.close()`
---
 .../retry/support/MetricsRetryListener.java   |  9 ++++----
 .../retry/support/RetryMetricsTests.java      | 23 +++++++++++++++----
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/src/main/java/org/springframework/retry/support/MetricsRetryListener.java b/src/main/java/org/springframework/retry/support/MetricsRetryListener.java
index 4dc9939e..92b9d0c6 100644
--- a/src/main/java/org/springframework/retry/support/MetricsRetryListener.java
+++ b/src/main/java/org/springframework/retry/support/MetricsRetryListener.java
@@ -66,8 +66,6 @@ public class MetricsRetryListener implements RetryListener {
 	private final Map<RetryContext, Timer.Sample> retryContextToSample = Collections
 		.synchronizedMap(new IdentityHashMap<>());
 
-	private final Timer.Builder retryMeterProvider;
-
 	private Tags customTags = Tags.empty();
 
 	private Function<RetryContext, Iterable<Tag>> customTagsProvider = retryContext -> Tags.empty();
@@ -79,7 +77,6 @@ public class MetricsRetryListener implements RetryListener {
 	public MetricsRetryListener(MeterRegistry meterRegistry) {
 		Assert.notNull(meterRegistry, "'meterRegistry' must not be null");
 		this.meterRegistry = meterRegistry;
-		this.retryMeterProvider = Timer.builder(TIMER_NAME).description("Metrics for Spring RetryTemplate");
 	}
 
 	/**
@@ -122,7 +119,11 @@ public <T, E extends Throwable> void close(RetryContext context, RetryCallback<T
 			.and(this.customTagsProvider.apply(context))
 			.and("exception", throwable != null ? throwable.getClass().getSimpleName() : "none");
 
-		sample.stop(this.retryMeterProvider.tags(retryTags).register(this.meterRegistry));
+		Timer.Builder timeBuilder = Timer.builder(TIMER_NAME)
+			.description("Metrics for Spring RetryTemplate")
+			.tags(retryTags);
+
+		sample.stop(timeBuilder.register(this.meterRegistry));
 	}
 
 }
diff --git a/src/test/java/org/springframework/retry/support/RetryMetricsTests.java b/src/test/java/org/springframework/retry/support/RetryMetricsTests.java
index 46985b4f..605697dc 100644
--- a/src/test/java/org/springframework/retry/support/RetryMetricsTests.java
+++ b/src/test/java/org/springframework/retry/support/RetryMetricsTests.java
@@ -16,6 +16,8 @@
 
 package org.springframework.retry.support;
 
+import java.util.concurrent.CompletableFuture;
+
 import io.micrometer.core.instrument.MeterRegistry;
 import io.micrometer.core.instrument.Tags;
 import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
@@ -27,6 +29,7 @@
 import org.springframework.retry.RetryException;
 import org.springframework.retry.annotation.EnableRetry;
 import org.springframework.retry.annotation.Retryable;
+import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor;
 import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -48,10 +51,20 @@ public class RetryMetricsTests {
 
 	@Test
 	void metricsAreCollectedForRetryable() {
-		assertThatNoException().isThrownBy(this.service::service1);
-		assertThatNoException().isThrownBy(this.service::service1);
-		assertThatNoException().isThrownBy(this.service::service2);
-		assertThatExceptionOfType(RetryException.class).isThrownBy(this.service::service3);
+		ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
+		executor.setCorePoolSize(4);
+		executor.afterPropertiesSet();
+
+		CompletableFuture<?> future1 = executor
+			.submitCompletable(() -> assertThatNoException().isThrownBy(this.service::service1));
+		CompletableFuture<?> future2 = executor
+			.submitCompletable(() -> assertThatNoException().isThrownBy(this.service::service1));
+		CompletableFuture<?> future3 = executor
+			.submitCompletable(() -> assertThatNoException().isThrownBy(this.service::service2));
+		CompletableFuture<?> future4 = executor.submitCompletable(
+				() -> assertThatExceptionOfType(RetryException.class).isThrownBy(this.service::service3));
+
+		CompletableFuture.allOf(future1, future2, future3, future4).join();
 
 		assertThat(this.meterRegistry.get(MetricsRetryListener.TIMER_NAME)
 			.tags(Tags.of("name", "org.springframework.retry.support.RetryMetricsTests$Service.service1", "retry.count",
@@ -70,6 +83,8 @@ void metricsAreCollectedForRetryable() {
 					"3", "exception", "RetryException"))
 			.timer()
 			.count()).isEqualTo(1);
+
+		executor.destroy();
 	}
 
 	@Configuration(proxyBeanMethods = false)