Thread race during FactoryBean instantiations starting with 6.2 due to lenient locks #35545
Description
Context
It seems like there were a lot of changes to the concurrency of singleton creations, here's my understanding of it:
- We have a deadlock issue Synchronization during singleton creation may result in deadlock #23501. To fix it we removed the mutex in 902e570:
- 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:
- We then add locking depending on threads in: fa168ca :
Problem
Now we get an issue when multiple threads start calling a @Lazy bean during startup, which then calls its FactoryBean in parallel, and if the factory is not thread safe there are issues.
In the case of #35242 it was actually the DiscoveryClient and InstanceInfoReplicator calling HealthIndicator instances in a background thread, which was in turn calling the feign client.
In our case we had something similar where a bean started a cron job with no delay.
Solution?
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, which was impossible before. From the original issue's description this is quite rare, and I would expect it to definitely be rarer than @Lazy FeignClients and other non thread safe factories:
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?
Here's a "reproduction"
// factory class
public class UnsafeFactoryBean implements FactoryBean<UnsafeFactoryBean.UnsafeBean> {
private final Phaser phaser = new Phaser(2);
@Override
public UnsafeBean getObject() {
System.out.println("Arrived");
// when two threads hit this, we proceed. With strict=true this never happens, so we hang
phaser.arriveAndAwaitAdvance();
return new UnsafeBean();
}
@Override
public Class<?> getObjectType() {
return UnsafeBean.class;
}
public static class UnsafeBean {
public void doSomething() {
System.out.println("doSomething");
}
}
}
// caller class
@Slf4j
public class UnsafeBeanCaller {
private final UnsafeFactoryBean.UnsafeBean unsafeBean;
private final ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();
public UnsafeBeanCaller(UnsafeFactoryBean.UnsafeBean unsafeBean) {
this.unsafeBean = unsafeBean;
check();
}
public void check() {
executorService.scheduleWithFixedDelay(() -> {
log.info("Checking unsafe bean...");
unsafeBean.doSomething();
}, 0, 5000, TimeUnit.MILLISECONDS);
}
}
// config
public class MyConfiguration {
@Bean
public UnsafeFactoryBean unsafeFactoryBean() {
return new UnsafeFactoryBean();
}
@Bean
public UnsafeBeanCaller lazyUnsafeBean(@Lazy UnsafeFactoryBean.UnsafeBean unsafeBean) {
return new UnsafeBeanCaller(unsafeBean);
}
@Bean
public UnsafeBeanCaller unsafeBean(@Lazy UnsafeFactoryBean.UnsafeBean unsafeBean) {
return new UnsafeBeanCaller(unsafeBean);
}
}