Synchronization during singleton creation may result in deadlock

[wilkinsona]

Affects: 5.1.9.RELEASE

During singleton creation, DefaultSingletonBeanRegistry synchronises on this.singletonObjects:

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

Line 204 in b1171d8

synchronized (this.singletonObjects) {

While synchronized, it then uses the singletonFactory to create the singleton:

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

Line 222 in b1171d8

singletonObject = singletonFactory.getObject();

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.

[nitinsh99]

I am seeing this exact same deadlock. Why are we synchronizing on a ConcurrentHashMap (singletonObjects) anyways?

[wilkinsona]

We've just seen this problem again in spring-projects/spring-boot#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();
			}
		}
		
	}

}

[jhoeller]

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.

[wilkinsona]

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.

[jhoeller]

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...

[wilkinsona]

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