From 40078f358d79cddd6c9761cc74014e28b333d51e Mon Sep 17 00:00:00 2001 From: jiangteng <1@1.cn> Date: Thu, 30 Apr 2026 17:03:36 +0800 Subject: [PATCH 1/2] Fix issue #1371 Signed-off-by: jiangteng <1@1.cn> --- .../support/FeignHttpMessageConverters.java | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConverters.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConverters.java index 07983183b..f12346606 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConverters.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConverters.java @@ -36,7 +36,7 @@ public class FeignHttpMessageConverters { private final ObjectProvider cloudCustomizers; - private List> converters; + private volatile List> converters; public FeignHttpMessageConverters(ObjectProvider customizers, ObjectProvider cloudCustomizers) { @@ -51,16 +51,21 @@ public List> getConverters() { private void initConvertersIfRequired() { if (this.converters == null) { - this.converters = new ArrayList<>(); - HttpMessageConverters.ClientBuilder builder = HttpMessageConverters.forClient(); - // TODO: allow disabling of registerDefaults - builder.registerDefaults(); - // TODO: check if already added? Howto order? + synchronized (this) { + if (this.converters == null) { + List> newConverters = new ArrayList<>(); + HttpMessageConverters.ClientBuilder builder = HttpMessageConverters.forClient(); + // TODO: allow disabling of registerDefaults + builder.registerDefaults(); + // TODO: check if already added? Howto order? - this.customizers.orderedStream().forEach(customizer -> customizer.customize(builder)); - HttpMessageConverters hmc = builder.build(); - hmc.forEach(converter -> converters.add(converter)); - cloudCustomizers.forEach(customizer -> customizer.accept(this.converters)); + this.customizers.orderedStream().forEach(customizer -> customizer.customize(builder)); + HttpMessageConverters hmc = builder.build(); + hmc.forEach(converter -> newConverters.add(converter)); + cloudCustomizers.forEach(customizer -> customizer.accept(newConverters)); + this.converters = newConverters; + } + } } } From 4b70bebd73e06ac14e288011d4d19b1b36ae7c96 Mon Sep 17 00:00:00 2001 From: jiangteng <1@1.cn> Date: Wed, 6 May 2026 15:44:08 +0800 Subject: [PATCH 2/2] Fix issue #1371: add tests Signed-off-by: jiangteng <1@1.cn> --- .../support/FeignHttpMessageConverters.java | 4 +- ...HttpMessageConvertersConcurrencyTests.java | 172 ++++++++++++++++++ 2 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConvertersConcurrencyTests.java diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConverters.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConverters.java index f12346606..31774b451 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConverters.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConverters.java @@ -38,6 +38,8 @@ public class FeignHttpMessageConverters { private volatile List> converters; + private final Object convertersLock = new Object(); + public FeignHttpMessageConverters(ObjectProvider customizers, ObjectProvider cloudCustomizers) { this.customizers = customizers; @@ -51,7 +53,7 @@ public List> getConverters() { private void initConvertersIfRequired() { if (this.converters == null) { - synchronized (this) { + synchronized (this.convertersLock) { if (this.converters == null) { List> newConverters = new ArrayList<>(); HttpMessageConverters.ClientBuilder builder = HttpMessageConverters.forClient(); diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConvertersConcurrencyTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConvertersConcurrencyTests.java new file mode 100644 index 000000000..920dc4a85 --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/FeignHttpMessageConvertersConcurrencyTests.java @@ -0,0 +1,172 @@ +/* + * Copyright 2013-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.cloud.openfeign.support; + +import org.junit.jupiter.api.Test; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +/** + * Tests for {@link FeignHttpMessageConverters} concurrency behavior. + * + * @author jiangteng + */ +class FeignHttpMessageConvertersConcurrencyTests { + @Test + void shouldInitializeConvertersOnlyOnceUnderConcurrentAccess() throws InterruptedException { + // Given + FeignHttpMessageConverters converters = new FeignHttpMessageConverters(mock(), mock()); + + int threadCount = 50; + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch endLatch = new CountDownLatch(threadCount); + List> results = new ArrayList<>(); + + // When - Multiple threads try to get converters simultaneously + for (int i = 0; i < threadCount; i++) { + executor.submit(() -> { + try { + startLatch.await(); // Wait for all threads to be ready + + // First call - triggers initialization + List firstResult = converters.getConverters(); + synchronized (results) { + results.add(firstResult); + } + + // Second call - should return cached result + List secondResult = converters.getConverters(); + assertThat(secondResult).isSameAs(firstResult); + } catch (Exception e) { + throw new RuntimeException(e); + } finally { + endLatch.countDown(); + } + }); + } + + // Release all threads at once to maximize contention + startLatch.countDown(); + + // Wait for all threads to complete + endLatch.await(); + executor.shutdown(); + + // Then - All threads should get the same converters instance + assertThat(results).hasSize(threadCount); + List firstConverters = results.get(0); + assertThat(firstConverters).isNotNull(); + assertThat(firstConverters).isNotEmpty(); + + // Verify all threads got the exact same instance (reference equality) + for (List result : results) { + assertThat(result).isSameAs(firstConverters); + } + } + + @Test + void shouldHandleRepeatedConcurrentAccess() throws InterruptedException { + // Given + FeignHttpMessageConverters converters = new FeignHttpMessageConverters(mock(), mock()); + + int threadCount = 100; + int iterationsPerThread = 100; + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch endLatch = new CountDownLatch(threadCount); + AtomicInteger errorCount = new AtomicInteger(0); + + // When - Multiple threads repeatedly access converters + for (int i = 0; i < threadCount; i++) { + executor.submit(() -> { + try { + startLatch.await(); + + List previous = null; + for (int j = 0; j < iterationsPerThread; j++) { + List current = converters.getConverters(); + if (previous != null) { + // Verify consistency - should always be the same instance + assertThat(current).isSameAs(previous); + } + previous = current; + } + } catch (AssertionError e) { + errorCount.incrementAndGet(); + throw e; + } catch (Exception e) { + errorCount.incrementAndGet(); + throw new RuntimeException(e); + } finally { + endLatch.countDown(); + } + }); + } + + startLatch.countDown(); + endLatch.await(); + executor.shutdown(); + + // Then - No errors should occur + assertThat(errorCount.get()).isEqualTo(0); + } + + @Test + void shouldNotExposeLockObject() throws InterruptedException { + // Given + FeignHttpMessageConverters converters = new FeignHttpMessageConverters(mock(), mock()); + CountDownLatch t1StartLatch = new CountDownLatch(1); + CountDownLatch t2StartLatch = new CountDownLatch(1); + + Thread t1 = new Thread(() -> { + try { + t1StartLatch.await(); + converters.getConverters(); + t2StartLatch.countDown(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + }); + + Thread t2 = new Thread(() -> { + try { + synchronized (converters) { + t1StartLatch.countDown(); + t2StartLatch.await(); + } + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + }); + + t2.start(); + t1.start(); + t1.join(1000); + t2.join(1000); + assertThat(t1.getState().equals(Thread.State.TERMINATED)).isTrue(); + assertThat(t2.getState().equals(Thread.State.TERMINATED)).isTrue(); + assertThat(converters.getConverters()).isNotNull(); + } +}