Feign client duplicates "path" parameter after upgrading to Spring Boot 3.4.6 from 3.3.10

[kptfh]

After upgrading Spring Boot from 3.3.10 to 3.4.6 and Spring Cloud from 2023.0.5 to 2024.0.1 we face the following errors in Production logs related to Feign client

Caused by: feign.RetryableException: Connection refused executing POST http://SERVICE-NAME/Path/Path/last-session

It was difficult to find the rootcause because this bug was flaky and happened on 1-2 instances out of 40.
After investigation it turned out that in spring-beans-6.2.7 there was a breaking change in instantiating singleton beans.
In new version locks for singleton beans were omitted for some cases and it leads to inconsistent behaviour of the service. In our case it was caused that some Feign clients were declared with @lazy and were not instantiated on startup.

So now to fix this issue we need to add spring.locking.strict=true to all our services (more then 400) to eliminate this breaking change.
It's not clear for our company (Playtika) why it wasn't set by default!

[kptfh]

OpenFeign/feign#1868

[bclozel]

I'm not aware of any problem in Spring Framework nor any change that could have caused this. I'm closing this now because I don't see anything actionable on our side.

[kptfh]

This commit in Spring Beans module introduces breaking change.

In method getObjectFromFactoryBean there is no strict lock now while instantiating singleton object not from main thread.
All @Lazy objects falls to this case.

If FactoryBean (like FeignClientFactoryBean ) is not thread-safe it may lead to incorrect beans instantiated.
FeignClientFactoryBean not thread safe

I just want to stress that all @Lazy beans with not thread-safe factories may be incorrectly instantiated now.
Taking into account the wide usage of @FeignClient the impact for services may be significant while upgrading to new Spring Boot.
It should be a good practice to keep code backward compatible so you need to set spring.locking.strict=true by default.

[bclozel]

@kptfh now that we know that this is not strictly related to feign, can you comment on #34902 then and explain the factorybean setup?

[kptfh]

You are right. It's not related to just Feign. It's related to all not thread-safe factory beans. And taking into account that there is no requirement to keep factory beans thread-safe, it potentially may affect a lot of beans (not Feign only).

The problem is that object = doGetObjectFromFactoryBean(factory, requiredType, beanName) is not protected by any locks now and in case of concurrent calls may lead to side effects in factory beans (bean factory may change it's state in unpredictable way).

Here is the config that leads to concurrent calls to FeignClientBeanFactory:

    @Bean
    public EvaluatorService evaluatorService(EvaluationClient client) {
        return new EvaluatorService(client);
    }

   @Bean
    public HealthIndicator clientWarmupHealthIndicator(@Lazy EvaluationClient client) {
        return new ClientWarmupHealthIndicator(client);
    }

I'm not sure if 34902 is related to my issue.

[kptfh]

@jhoeller Hi!
can you check and reopen this issue?

[dpozinen]

Hi @jhoeller We have encountered this issue as well.
It seems like there were a lot of changes to the concurrency of singleton creations, here's my understanding of it:

1. We have a deadlock issue Synchronization during singleton creation may result in deadlock #23501. To fix it we removed the mutex in 902e570:
   

   spring-framework/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java

   Lines 992 to 994 in 99a366b

   	private FactoryBean<?> getSingletonFactoryBeanForTypeCheck(String beanName, RootBeanDefinition mbd) {
   	synchronized (getSingletonMutex()) {
   	BeanWrapper bw = this.factoryBeanInstanceCache.get(beanName);

2. We have no locks at all and that caused BeanCurrentlyInCreationException is thrown when multiple threads simultaneously try to create a FactoryBean #33972. So, we added a lock in 384dc2a:
   

   spring-framework/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java

   Lines 993 to 994 in 384dc2a

   	private FactoryBean<?> getSingletonFactoryBeanForTypeCheck(String beanName, RootBeanDefinition mbd) {
   	this.singletonLock.lock();

3. Now as far as I can see we are essentially where we started? Unless there is some nuance in what we were synchronizing against vs locking, but irregardless.
   Also, a different deadlock issue is reported: Deadlock with background EntityManager bootstrap due to locking in getSingletonFactoryBeanForTypeCheck #34247. So we fix that by not actually waiting for the lock in 8771b9e:
   

   spring-framework/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java

   Lines 993 to 997 in 8771b9e

   	private FactoryBean<?> getSingletonFactoryBeanForTypeCheck(String beanName, RootBeanDefinition mbd) {
   	boolean locked = this.singletonLock.tryLock();
   	if (!locked) {
   	return null;
   	}

4. Now, (cmiiw) since were are not locking, some beans are just considered "missing" and hence they are not injected as reported in Beans created by FactoryBean are not considered as autowiring candidates if another thread holds a singletonLock #34902 . We fix this by locking depending on threads in: fa168ca

I don't have a deep enough understanding to challenge the thread logic, but now we definitely have the issue of non thread-safe FactoryBean implementations causing hard to debug issues. One of them is the FeignClientFactoryBean , we had our own problematic implementation also.

In my opinion strict locking should be the enabled by default, and if I understand correctly we would (broadly) arrive at state №1. But for those experiencing deadlocks they can disable strict locking. From the original issue's description this is quite rare, and I would expect it to definitely be rarer than @Lazy FeignClients:

It's tricky and quite complex. In some situations, listening to the GC notification won't cause any bean creation. It will cause bean creation if you're using Prometheus, have Exemplars enabled, and the lazily created SpanContextSupplier implementation hasn't already been created

What do you think about changing the default to strict? If you want to change it for optimization purposes, maybe we can have a grace period where spring requires thread-safe FactoryBean-s and then use non strict as default?