AbstractApplicationContext.refresh() leaves bean resources uncleaned when SmartInitializationSingleton throws a non BeansException

[pfoermer]

Affects: Tested with 5.3.22 and 5.2.20.RELEASE

Problem description:
For some configuration issues during application context creation the AbstractApplicationContext.refresh()
method is not executing the catch(BeansException ex)/clean block. In this cleanup-block already created beans are getting destroyed by calling their close method. Especially for JUnit 5 annotated classes with @ExtendWith(SpringExtension.class)
this may cause potential resource leaks as for every test method depending on a faulty context the context will be tried to recreated without previous clean-up of resources which stay alive.

In a real world project the faulty, not destroyed, test contexts consumed all database connections of a shared DBMS as the created Hikari CP DataSource Pool was not closed/freed by AbstractApplicationContext.refresh().
At the end the amount of not cleaned up test contexts also caused an OutOfMemoryError as Hikari CP makes use of background threads and so references survive the garbage collector for the no longer used AbstractApplicationContext.

I attached a sample project
test-app.zip with
two "test samples/cases" (src/test/java/app) which will both cause a not successful application context creation but with two different behaviours of the clean-up:

1. For "LeakTest" the close method of "DummyDbcp" will not be called by AbstractApplicationContext, also the bean was created in the application context. The text "Pool closed, db connections released" is not printed to STDOUT via logger.
2. For "NonCausingLeakTest" the close method of "DummyDbcp" will be called by AbstractApplicationContext, preventing against resource leaks. The text "Pool closed, db connections released" is printed to STDOUT via logger.

The pom.xml of the project makes use of spring-boot-starter's, but only spring-framework components are really used.

Best regards,
Philipp Förmer

[snicoll]

Thanks for the sample and sorry it got overlooked thus far.

@jhoeller I am a bit confused as why already-created beans would not be closed on a failed context. I like the sample as it's bare bone but I wonder if I am missing something. Perhaps a companion to #20028?

[snicoll]

So it turns out to be quite an interesting review of AbstractApplicationContext#refresh. Refresh on catch BeansException as this is what it's expecting anything that would be thrown as part of the context refresh would have been wrapped in an exception of that type, except the afterSingletonInstantied callback.

Discussing with @jhoeller we were wondering if doing that there would be an option but, in retrospect, we think changing the catch block to handle any RuntimeException would be much future proof and will make sure that beans that have already been created to be reclaimed.

Thanks again for the sample @pfoermer