From fa168ca78ae134e82db8eacc109bb29266b36fb1 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 15 May 2025 01:45:09 +0200 Subject: [PATCH] Revise FactoryBean locking behavior for strict/lenient consistency After the bootstrap phase (and with spring.locking.strict=true during the bootstrap phase), getSingletonFactoryBeanForTypeCheck always locks. In a background bootstrap thread, it never locks. Otherwise, it tries locking and explicitly resolves the bean class for subsequent type-based resolution (even for a component-scanned class) when it fails to acquire the lock. Furthermore, getObjectFromFactoryBean follows the same locking algorithm for post-processing. Closes gh-34902 --- .../AbstractAutowireCapableBeanFactory.java | 14 +++++++-- .../support/DefaultSingletonBeanRegistry.java | 12 ++++---- .../support/FactoryBeanRegistrySupport.java | 30 ++++++++++++++----- .../annotation/BackgroundBootstrapTests.java | 15 ++++++++-- 4 files changed, 53 insertions(+), 18 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java index a46a36c66d3..38887a61a91 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java @@ -997,9 +997,17 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac */ @Nullable private FactoryBean getSingletonFactoryBeanForTypeCheck(String beanName, RootBeanDefinition mbd) { - boolean locked = this.singletonLock.tryLock(); - if (!locked) { - return null; + Boolean lockFlag = isCurrentThreadAllowedToHoldSingletonLock(); + if (lockFlag == null) { + this.singletonLock.lock(); + } + else { + boolean locked = (lockFlag && this.singletonLock.tryLock()); + if (!locked) { + // Avoid shortcut FactoryBean instance but allow for subsequent type-based resolution. + resolveBeanClass(mbd, beanName); + return null; + } } try { diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java index ad3ec147bd5..a5f8585bc89 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java @@ -271,13 +271,15 @@ public class DefaultSingletonBeanRegistry extends SimpleAliasRegistry implements // Fallback as of 6.2: process given singleton bean outside of singleton lock. // Thread-safe exposure is still guaranteed, there is just a risk of collisions // when triggering creation of other beans as dependencies of the current bean. - if (logger.isInfoEnabled()) { - logger.info("Obtaining singleton bean '" + beanName + "' in thread \"" + - Thread.currentThread().getName() + "\" while other thread holds " + - "singleton lock for other beans " + this.singletonsCurrentlyInCreation); - } this.lenientCreationLock.lock(); try { + if (logger.isInfoEnabled()) { + Set lockedBeans = new HashSet<>(this.singletonsCurrentlyInCreation); + lockedBeans.removeAll(this.singletonsInLenientCreation); + logger.info("Obtaining singleton bean '" + beanName + "' in thread \"" + + currentThread.getName() + "\" while other thread holds singleton " + + "lock for other beans " + lockedBeans); + } this.singletonsInLenientCreation.add(beanName); } finally { diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/FactoryBeanRegistrySupport.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/FactoryBeanRegistrySupport.java index ffcd87bbfbc..2ba14e3484d 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/FactoryBeanRegistrySupport.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/FactoryBeanRegistrySupport.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 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. @@ -118,7 +118,15 @@ public abstract class FactoryBeanRegistrySupport extends DefaultSingletonBeanReg */ protected Object getObjectFromFactoryBean(FactoryBean factory, String beanName, boolean shouldPostProcess) { if (factory.isSingleton() && containsSingleton(beanName)) { - this.singletonLock.lock(); + Boolean lockFlag = isCurrentThreadAllowedToHoldSingletonLock(); + boolean locked; + if (lockFlag == null) { + this.singletonLock.lock(); + locked = true; + } + else { + locked = (lockFlag && this.singletonLock.tryLock()); + } try { Object object = this.factoryBeanObjectCache.get(beanName); if (object == null) { @@ -131,11 +139,13 @@ public abstract class FactoryBeanRegistrySupport extends DefaultSingletonBeanReg } else { if (shouldPostProcess) { - if (isSingletonCurrentlyInCreation(beanName)) { - // Temporarily return non-post-processed object, not storing it yet - return object; + if (locked) { + if (isSingletonCurrentlyInCreation(beanName)) { + // Temporarily return non-post-processed object, not storing it yet + return object; + } + beforeSingletonCreation(beanName); } - beforeSingletonCreation(beanName); try { object = postProcessObjectFromFactoryBean(object, beanName); } @@ -144,7 +154,9 @@ public abstract class FactoryBeanRegistrySupport extends DefaultSingletonBeanReg "Post-processing of FactoryBean's singleton object failed", ex); } finally { - afterSingletonCreation(beanName); + if (locked) { + afterSingletonCreation(beanName); + } } } if (containsSingleton(beanName)) { @@ -155,7 +167,9 @@ public abstract class FactoryBeanRegistrySupport extends DefaultSingletonBeanReg return object; } finally { - this.singletonLock.unlock(); + if (locked) { + this.singletonLock.unlock(); + } } } else { diff --git a/spring-context/src/test/java/org/springframework/context/annotation/BackgroundBootstrapTests.java b/spring-context/src/test/java/org/springframework/context/annotation/BackgroundBootstrapTests.java index 75f446f6ad3..ed5e45b85b8 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/BackgroundBootstrapTests.java +++ b/spring-context/src/test/java/org/springframework/context/annotation/BackgroundBootstrapTests.java @@ -24,6 +24,7 @@ import org.junit.jupiter.api.Timeout; import org.springframework.beans.factory.BeanCreationException; import org.springframework.beans.factory.BeanCurrentlyInCreationException; +import org.springframework.beans.factory.FactoryBean; import org.springframework.beans.factory.ObjectProvider; import org.springframework.beans.factory.UnsatisfiedDependencyException; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; @@ -243,14 +244,24 @@ class BackgroundBootstrapTests { } @Bean - public TestBean testBean4() { + public FactoryBean testBean4() { try { Thread.sleep(2000); } catch (InterruptedException ex) { Thread.currentThread().interrupt(); } - return new TestBean(); + TestBean testBean = new TestBean(); + return new FactoryBean<>() { + @Override + public TestBean getObject() { + return testBean; + } + @Override + public Class getObjectType() { + return testBean.getClass(); + } + }; } }