spring-framework
spring-framework copied to clipboard
Synchronization during singleton creation may result in deadlock
Affects: 5.1.9.RELEASE
During singleton creation, DefaultSingletonBeanRegistry
synchronises on this.singletonObjects
:
https://github.com/spring-projects/spring-framework/blob/b1171d82967cfd374f0b08f9580fe662fa1a98e8/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java#L204
While synchronized, it then uses the singletonFactory
to create the singleton:
https://github.com/spring-projects/spring-framework/blob/b1171d82967cfd374f0b08f9580fe662fa1a98e8/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultSingletonBeanRegistry.java#L222
This call into user code while holding a lock can result in deadlock. We've seen one example reported in this Spring Boot issue where Micrometer is also involved. I've also reproduced a very similar problem without Micrometer and with no synchronization in user code:
package example;
import javax.annotation.PostConstruct;
import javax.validation.Validator;
import javax.validation.constraints.Max;
import org.junit.jupiter.api.Test;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Lazy;
import org.springframework.validation.annotation.Validated;
import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean;
import org.springframework.validation.beanvalidation.MethodValidationPostProcessor;
public class SingletonCreationDeadlockTests {
@Test
public void create() {
new AnnotationConfigApplicationContext(Config.class).close();;
}
private static final class Registry {
private final ConfigProperties properties;
Registry(ConfigProperties properties) {
this.properties = properties;
}
void register() {
this.properties.getSetting();
}
}
@Validated
static class ConfigProperties {
@Max(10)
private int setting = 5;
public int getSetting() {
return this.setting;
}
public void setSetting(int setting) {
this.setting = setting;
}
}
@Configuration
static class Config {
@Bean
public Registry registry(ConfigProperties properties) {
return new Registry(properties);
}
@Bean
public ConfigProperties properties() {
return new ConfigProperties();
}
@Bean
public LocalValidatorFactoryBean localValidatorFactoryBean() {
return new LocalValidatorFactoryBean();
}
@Bean
public static MethodValidationPostProcessor methodValidationPostProcessor(@Lazy Validator validator) {
MethodValidationPostProcessor postProcessor = new MethodValidationPostProcessor();
postProcessor.setValidator(validator);
return postProcessor;
}
@Bean
public Registrar registrar(Registry registry) {
return new Registrar(registry);
}
}
static class Registrar {
private final Registry registry;
Registrar(Registry registry) {
this.registry = registry;
}
@PostConstruct
void register() {
Thread thread = new Thread(() -> {
registry.register();
});
thread.start();
try {
thread.join();
} catch (InterruptedException ex) {
Thread.currentThread().interrupt();
}
}
}
}
Here's a zip of a complete project containing the above test: singleton-creation-deadlock.zip
The deadlock occurs because the main thread has locked singletonObjects
and then waits for the thread created by Registrar
to complete. The thread created by Registrar
ends up waiting to lock singletonObjects
due to ConfigProperties
being @Validated
and the resolution of the @Lazy
Validator
requiring a call to DefaultListableBeanFactory.doResolveDependency
which results in a call to DefaultSingletonBeanRegistry.getSingleton
where the attempt to lock singletonObjects
is made.
I am seeing this exact same deadlock. Why are we synchronizing
on a ConcurrentHashMap (singletonObjects)
anyways?
We've just seen this problem again in https://github.com/spring-projects/spring-boot/issues/33070. Here's a slightly simpler recreation that uses an ObjectProvider
rather than a @Lazy
Validator
to trigger the problem:
package com.example.demo;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.ObjectProvider;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import jakarta.annotation.PostConstruct;
public class SingletonCreationDeadlockTests {
@Test
public void create() {
new AnnotationConfigApplicationContext(Config.class).close();;
}
private static final class Registry {
private final ObjectProvider<ConfigProperties> properties;
Registry(ObjectProvider<ConfigProperties> properties) {
this.properties = properties;
}
void register() {
this.properties.getIfAvailable().getSetting();
}
}
static class ConfigProperties {
private int setting = 5;
public int getSetting() {
return this.setting;
}
public void setSetting(int setting) {
this.setting = setting;
}
}
@Configuration
static class Config {
@Bean
public Registry registry(ObjectProvider<ConfigProperties> properties) {
return new Registry(properties);
}
@Bean
public Registrar registrar(Registry registry) {
return new Registrar(registry);
}
@Bean
public ConfigProperties properties() {
return new ConfigProperties();
}
}
static class Registrar {
private final Registry registry;
Registrar(Registry registry) {
this.registry = registry;
}
@PostConstruct
void register() {
Thread thread = new Thread(() -> {
registry.register();
});
thread.start();
try {
thread.join();
} catch (InterruptedException ex) {
Thread.currentThread().interrupt();
}
}
}
}
The problem here is the starting of a new thread in @PostConstruct
which is conceptually a variant of InitializingBean.afterPropertiesSet
, only really meant to validate configuration state before publishing the bean instance to the container (which is why it has happening within the singleton creation lock). It's a bit like the end of a constructor implementation.
Doing extensive work that might trigger new threads - and even wait for them to return - is rather meant to happen in a SmartInitializingSingleton.afterSingletonsInstantiated
callback (or in an ApplicationListener<ContextRefreshedEvent>
or the like). For a simple guideline: Whatever you would not do in a constructor, you should not do in a @PostConstruct
method either.
Revisiting the singleton creation lock is a tough challenge due to singleton beans typically being part of a larger bean dependency graph. With a relaxed per-bean lock, circular references might run into a deadlock when triggered from different threads. There is no simple works-for-everything solution here, I'm afraid.
Thanks, Juergen. The new thread started in @PostConstruct
was just a synthetic way of recreating the deadlock. In the latest situation where we've seen the problem there's no user code starting or joining a thread.
The two threads that are involved are the main thread and the JVM's "Notification Thread". Due to Micrometer listening for GC notifications, the notification thread is making a call to a SingletonSupplier
that's calling ObjectProvider.getObject()
to supply the singleton. This results in it waiting to take the singleton creation lock. At the same time, the main thread is holding the singleton creation lock and then ends up trying to call the same SingletonSupplier
which is locked by the notification thread.
Thanks for the clarification, Andy - that clarifies a lot.
Could Micrometer possibly only start listening to GC notifications once it is fully initialized, including the beans that it depends on? It seems brittle to let GC notifications trigger any kind of bean initialization to begin with...
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. The laziness is necessary as there's a dependency cycle otherwise:
/**
* Since the MeterRegistry can depend on the {@link Tracer} (Exemplars) and the
* {@link Tracer} can depend on the MeterRegistry (recording metrics), this
* {@link SpanContextSupplier} breaks the cycle by lazily loading the {@link Tracer}.
*/
I take the general point though that in all likelihood we need to find a way to resolve this in Boot and/or Micrometer.
/cc @jonatan-ivanov