No early bean initialization from `EmbeddedKafkaContextCustomizer`

[garyrussell]

See spring-projects/spring-framework#31453 (comment)

While we don't support the embedded broker in native images; the problem seems to be that the broker is not shut down properly.

[snicoll]

The problem is rather that the broker is starting "manually" as part of a callback that should not start any bean, see

spring-kafka/spring-kafka-test/src/main/java/org/springframework/kafka/test/context/EmbeddedKafkaContextCustomizer.java

Lines 127 to 128 in 0dfde15

	beanFactory.initializeBean(embeddedKafkaBroker, EmbeddedKafkaBroker.BEAN_NAME);
	beanFactory.registerSingleton(EmbeddedKafkaBroker.BEAN_NAME, embeddedKafkaBroker);

Customize the supplied ConfigurableApplicationContext after bean definitions have been loaded into the context but before the context has been refreshed.

I think it should rather contribute a RootBeanDefinition. Unfortunately, the contract of this method is a bit strange as it should provide a registry, not a context since it has not been refreshed yet.

Here is an example in Spring Boot of what I mean: https://github.com/spring-projects/spring-boot/blob/8f2ec227389391fdd173db0ab64f26abd2752f20/spring-boot-project/spring-boot-test/src/main/java/org/springframework/boot/test/web/client/TestRestTemplateContextCustomizer.java#L62

[artembilan]

Just debugged one of the test using @EmbeddedKafka. The EmbeddedKafkaZKBroker.destroy() is called and all the brokers and Zookeeper are shouted down in normal JVM.
I fail to find that @DisabledInAotMode to mark our @EmbeddedKafka, but I see that mentioned TestRestTemplateContextCustomizer uses if (AotDetector.useGeneratedArtifacts()) {.
Maybe that would be enough for us as well.
At the same time I don't see too much difference with what we have so far with a ((DefaultSingletonBeanRegistry) beanFactory).registerDisposableBean(EmbeddedKafkaBroker.BEAN_NAME, embeddedKafkaBroker); and what Spring Boot does:

	if (beanFactory instanceof BeanDefinitionRegistry registry) {
			registerTestRestTemplate(registry);
		}

Even if we are about migrating to the RootBeanDefinition, we still need a BeanDefinitionRegistry registry.
So, I don't understand what we are pursuing here.

Thanks

[snicoll]

Again, this isn't a problem with shutdown. It's not even a problem with AOT actually even if it surfaces only when one tries to process the test context using Spring AOT.

I don't see too much difference with what we have [...] and what Spring Boot does

There's a huge difference. Spring Kafka creates a singleton and force start it manually at a time where only the context can be customized before the refresh (see javadoc). Spring Boot registers a bean definition that's going to be handled just like any other bean at the appropriate time.

Even if we are about migrating to the RootBeanDefinition, we still need a BeanDefinitionRegistry registry.

Correct and there's nothing in the issue I can see that's asking to get a way from casting the registry. I've tried to explain that with "Unfortunately, the contract of this method is a bit strange as it should provide a registry, not a context since it has not been refreshed yet.".

Within customizeContext you should not start any bean as this isn't the appropriate time to do so. The ask is to change the implementation so that it stops doing that.

[artembilan]

Still not clear why is that harmful.
The EmbeddedKafkaBroker instance does not have any dependencies, therefore that beanFactory.initializeBean(embeddedKafkaBroker, EmbeddedKafkaBroker.BEAN_NAME); is thing by itself just to call its afterPropertiesSet() to start embedded brokers before the real test suite.
And therefore changing it to the:

((BeanDefinitionRegistry) beanFactory).registerBeanDefinition(EmbeddedKafkaBroker.BEAN_NAME, 
				new RootBeanDefinition(EmbeddedKafkaBroker.class, () -> embeddedKafkaBroker));

won't have too much difference for the end result.
(Assuming that we agreed we don't need to rework it to plain BeanDefinition configuration since no AOT support for this type of tests.)

[garyrussell]

The difference is initializeBean() creates the broker immediately, whereas registering a bean definition defers the creation until the context is refreshed.

[snicoll]

Still not clear why is that harmful.

Because the code starts a bean at a time where the ApplicationContext is configured. It should not do that and this has nothing to do with AOT.