@SpringBootTest with CommonsPool2TargetSource hangs indefinitely after update to 2.4.x

[skorhone]

Some of our tests started to hang after we updated to 2.4.0. We narrowed down error to SpringBootMockResolver

It looks like that bean retrieved by SpringBootMockResolver is never released back to pool. For pooled beans this means that pool will eventually run out and tests freeze.

You can find example here: https://github.com/skorhone/spring-boot-test-mockito-bug

[wilkinsona]

See #22416 which introduced the mock resolver.

[wilkinsona]

Thanks for the report. The way that Mockito uses the configured MockResolver means that there's no point at which we can release the target. This means that it isn't safe for us to retrieve a target from a non-static source so I think we'll have to ignore proxies with such a target source.

That can be done with a mock resolver like this:

/*
 * Copyright 2012-2020 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.
 * You may obtain a copy of the License at
 *
 *      https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package org.springframework.boot.test.mock.mockito;

import org.mockito.plugins.MockResolver;

import org.springframework.aop.TargetSource;
import org.springframework.aop.framework.Advised;
import org.springframework.aop.support.AopUtils;
import org.springframework.test.util.AopTestUtils;
import org.springframework.util.Assert;

/**
 * A {@link MockResolver} for testing Spring Boot applications with Mockito. Resolves
 * mocks in a manner similar to {@link AopTestUtils#getUltimateTargetObject(Object)},
 * but stopping when a proxy with a {@link TargetSource#isStatic() non-static} {@link TargetSource}
 * is encountered.
 *
 * @author Andy Wilkinson
 * @since 2.4.0
 */
public class SpringBootMockResolver implements MockResolver {

	@Override
	public Object resolve(Object instance) {
		return getUltimateStaticTargetObject(instance);
	}

	@SuppressWarnings("unchecked")
	public static <T> T getUltimateStaticTargetObject(Object candidate) {
		Assert.notNull(candidate, "Candidate must not be null");
		try {
			if (AopUtils.isAopProxy(candidate) && candidate instanceof Advised) {
				TargetSource targetSource = ((Advised) candidate).getTargetSource();
				if (targetSource.isStatic()) {
					Object target = targetSource.getTarget();
					if (target != null) {
						return (T) getUltimateStaticTargetObject(target);
					}
				}
			}
		}
		catch (Throwable ex) {
			throw new IllegalStateException("Failed to unwrap proxied object", ex);
		}
		return (T) candidate;
	}

}

You can work around the problem by copying the above class into src/test/java, ensuring that it remains in the org.springframework.boot.test.mock.mockito package so that it overwrites Boot's class with the same name.

I've reached out to the Framework team to see if methods that avoid drilling down into non-static sources could be added to AopTestUtils or if we should handle this entirely in Boot.

[skorhone]

Thank you.

We were actually able to circumvent the issue with following changes.

First we removed bean that uses proxyFactory directly:

	@Bean(name = "demoProxy")
	public DemoBean demoProxy(@Qualifier(ID) ProxyFactoryBean federationProxyFactoryBean) {
		return (DemoBean) federationProxyFactoryBean.getObject();
	}

And then modified annotation for proxy factory to include name:

    @Bean(name = "demoProxy")
    @Qualifier(ID)
    public ProxyFactoryBean proxyFactoryBean(@Qualifier(ID) CommonsPool2TargetSource targetSource) {
        ProxyFactoryBean p = new ProxyFactoryBean();
        p.setTargetSource(targetSource);
        p.setProxyTargetClass(true);
        return p;
    }    

I assume code fragment that we earlier used was a copy+paste. One of top matches for "spring boot bean pooling" uses the same pattern.

[wilkinsona]

Removing the bean that uses proxyFactory directly is what avoids the problem. That means that there's no longer a singleton bean that is using a pooled target source for its proxying.

[wilkinsona]

Thinking about this some more, I think the problem is due to user error. A singleton bean that uses a pooled target source will permanently borrow an item from the pool which defeats the purpose of the pooling. We already have logic in our code that resets mock to only process singleton beans:

spring-boot/spring-boot-project/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/ResetMocksTestExecutionListener.java

Line 79 in dcae55a

if (definition.isSingleton() && instantiatedSingletons.contains(name)) {

With beans using a pooled target source correctly defined as non-singleton beans, the problem will not occur.

[skorhone]

@wilkinsona I do agree that it doesn't make any sense to create that singleton bean. However, the bean doesn't actually behave like you expected. It doesn't borrow object from pool.

On 2.3.2.RELEASE following code:

	@Test
	void first() throws Exception {
		ExecutorService es = Executors.newFixedThreadPool(5);
		for (int i = 0;i < 100;i++) {
			es.submit(() -> System.out.println(getDemoBean().getId()));
		}
		es.shutdown();
		es.awaitTermination(10, TimeUnit.SECONDS);
	}

Will output:

0e2138d5-aa29-487f-8cb1-33dc24cd3c59
e22032d8-88a0-4337-8c7f-a50c108c44b1
0e2138d5-aa29-487f-8cb1-33dc24cd3c59
e22032d8-88a0-4337-8c7f-a50c108c44b1
0e2138d5-aa29-487f-8cb1-33dc24cd3c59
e22032d8-88a0-4337-8c7f-a50c108c44b1
e22032d8-88a0-4337-8c7f-a50c108c44b1
...

If I'm not mistaken, the singleton bean points to a proxy, that borrows a bean from pool for each method invocation.

[wilkinsona]

You're right. Thanks for the correction. I'm still leaning towards not making a change here as I'd prefer not to add the extra complexity unless it's really needed.