Propagation REQUIRES_NEW may cause connection pool deadlock

[wlizhi]

A method that opens a transaction calls another method that starts a new transaction，if all connections are exhausted before the last new transaction method is executed,then all threads in the process will block，this process will fail.

Pseudo code：

@Transactional
methodA(){
    // All the threads that started the transaction were executed here, but the connection was exhausted.
    // The latter method execution will get a new connection,but it will never get it.
    @Transactional(propagation=Propagation.REQUIRES_NEW)
    methodB(){
    }
}

[quaff]

What's the problem?

[wlizhi]

Propagation.REQUIRES_NEW May cause all connections to be occupied.if this happens, the entire process will be blocked and unable to provide services

[wlizhi]

When all connections in the connection pool are occupied by the transaction method that does not execute to the last newly started transaction, all threads cannot obtain new connections, and the whole process will be blocked.

[quaff]

Do you know REQUIRES_NEW will double connections?

[wlizhi]

REQUIRES_NEW is not_ New will occupy two connections, but this transaction propagation behavior will suspend the previous transaction and restart a transaction. When a new transaction is opened, a new connection must be obtained. The previously suspended transaction connection will not be released.

[quaff]

It's by design, If first connection released, the outer transaction cannot continue.

[wlizhi]

if (definition.getPropagationBehavior() == TransactionDefinition.PROPAGATION_REQUIRES_NEW) {
if (debugEnabled) {
logger.debug("Suspending current transaction, creating new transaction with name [" +
definition.getName() + "]");
}
SuspendedResourcesHolder suspendedResources = suspend(transaction);
try {
return startTransaction(definition, transaction, debugEnabled, suspendedResources);
}
catch (RuntimeException | Error beginEx) {
resumeAfterBeginException(transaction, suspendedResources, beginEx);
throw beginEx;
}
}

[wlizhi]

The above is the source code. When a new transaction is opened, the previous transaction will be suspended, so that the peripheral methods will occupy multiple connections during the execution process. Suppose that there are 50 connections in the database connection pool. At this time, 50 threads are just executing to the peripheral methods. They are all preparing to open new transactions and obtain new connections, but they will never get them because the connections have been exhausted.

[wlizhi]

After this happens, all connections in the process may be blocked.When this happens, all threads that will start the transaction will be blocked.

[wlizhi]

My question is, this way is likely to cause the whole process to block, so what is the significance of it?

[quaff]

As I said, if connection is released, the outer transaction cannot be resumed.
https://github.com/spring-projects/spring-framework/blob/master/spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java#L604-L647

[wlizhi]

I know that if the connection has been released, the external transaction cannot be resumed. But REQUIRES_NEW may cause the whole process to hang up. So what's the point of it?

[quaff]

I explained why REQUIRES_NEW will double connections, If your connection pool is exhausted, you should increase max pool size, or handle the concurrency, for example by using Bulkhead of resilience4j, it's not responsibility of spring transaction.
I didn't get your point, do you think there is a bug or improvement here? If you want ask question, please visit stackoverflow.

[wlizhi]

@service
public class PropagationExampleServiceImpl implements PropagationExampleService {
@Autowired
PropagationExampleService propagationExampleService;

@Transactional
@Override
public void methodA() throws InterruptedException {
	Thread.sleep(100);
            // Assuming that multiple threads are executed here, all connections in the connection pool are used to open 
           // transactions of peripheral methods. Then they will never be released.
	propagationExampleService.methodB();
}

@Transactional(propagation = Propagation.REQUIRES_NEW)
@Override
public void methodB() throws InterruptedException {
	Thread.sleep(100);
}

}

[wlizhi]

Nested new start transactions cannot be used in actual production environments. It is likely that all connections are occupied when executing to the peripheral transaction method, and then blocked at the method of the newly started inner transaction.

[wlizhi]

I think there is something wrong with the design of REQUIRES_NEW.

[quaff]

It's not forum here, if you think it's a bug please paste test case, if you think it's an improvement please submit PR, end of discussion.

[wlizhi]

Sorry, I didn't make it clear. This is the test code, where it blocks the real process. Methodb() will never execute.

@slf4j
@service
public class PropagationExampleServiceImpl implements PropagationExampleService {
@Autowired
PropagationExampleService service;

@Transactional
@Override
public void methodA() throws InterruptedException {
	Thread.sleep(100);
	log.info("methodA");
	service.methodB();
}

@Transactional(propagation = Propagation.REQUIRES_NEW)
@Override
public void methodB() throws InterruptedException {
	log.info("methodB");
	Thread.sleep(100);
}

}

@SpringBootTest
public class PropagationExampleServiceTest {
@Autowired
PropagationExampleService service;
ExecutorService executor = new ThreadPoolExecutor(10, 10, 60,
TimeUnit.SECONDS, new ArrayBlockingQueue<>(1000));

@Test
public void methodATest() {
	for (int i = 0; i < 20; i++) {
		executor.execute(() -> {
			try {
				service.methodA();
			} catch (InterruptedException e) {
				e.printStackTrace();
			}
		});
	}
	LockSupport.park();
}

}

spring:
datasource:
name: druidDataSource
type: com.alibaba.druid.pool.DruidDataSource
druid:
max-active: 10

Here is the log:
2020-12-11 10:15:56.385 INFO 10864 --- [pool-1-thread-1] c.j.s.s.i.PropagationExampleServiceImpl : methodA
2020-12-11 10:15:56.385 INFO 10864 --- [pool-1-thread-2] c.j.s.s.i.PropagationExampleServiceImpl : methodA
2020-12-11 10:15:56.385 INFO 10864 --- [pool-1-thread-5] c.j.s.s.i.PropagationExampleServiceImpl : methodA
2020-12-11 10:15:56.385 INFO 10864 --- [pool-1-thread-6] c.j.s.s.i.PropagationExampleServiceImpl : methodA
2020-12-11 10:15:56.385 INFO 10864 --- [pool-1-thread-8] c.j.s.s.i.PropagationExampleServiceImpl : methodA
2020-12-11 10:15:56.385 INFO 10864 --- [pool-1-thread-9] c.j.s.s.i.PropagationExampleServiceImpl : methodA
2020-12-11 10:15:56.385 INFO 10864 --- [pool-1-thread-3] c.j.s.s.i.PropagationExampleServiceImpl : methodA
2020-12-11 10:15:56.385 INFO 10864 --- [ool-1-thread-10] c.j.s.s.i.PropagationExampleServiceImpl : methodA
2020-12-11 10:15:56.385 INFO 10864 --- [pool-1-thread-7] c.j.s.s.i.PropagationExampleServiceImpl : methodA
2020-12-11 10:15:56.385 INFO 10864 --- [pool-1-thread-4] c.j.s.s.i.PropagationExampleServiceImpl : methodA

[quaff]

I think the result is expected, you should increase max-active > concurrency, so If you change max-active to 21, it will pass, waiting for your confirmation.
You should set a max-wait on dataSource, then getConnection will timeout instead of endless blocking.

[wlizhi]

I know, I just want to express that. In fact, the number of connections in the connection pool is generally not large. In the above example, only one layer is nested. If there are multiple layers of newly started transactions in the code, only a small amount of concurrency is needed to trigger the above result.

I just think that when multiple threads compete for resources, each thread must grab multiple resources before ending the task, which may be a hidden danger.

Because this method may cause multiple threads to rob only a part of the resources they need. They can't release what they have acquired without getting all the resources they need. By this time, however, resources have been exhausted.

[quaff]

Your test blocked because you didn't set timeout for ds.getConnection(), for example maxWait for druid and connectionTimeout for HikariCP.
It's really nothing to do with spring transaction.

[wlizhi]

I'm sorry to have taken you so long.

I just want to put forward what I think may be hidden dangers. It's not that I have encountered problems in use. I just don't quite understand why this place is designed in this way.

Even if Max wait is set here, the above situation may occur. It's just going to wait for a while, not the whole process, but it's bad enough.

For this reason, I choose to avoid using REQUIRES_NEW, if I need to start a new transaction in a method, I will use asynchronous or transactionSynchronization.afterCommit ()。

You may say that the outer method will not be able to sense the exception, but why should it? These are two transactions. If I need the outer layer to sense the inner layer's anomaly, I can use NESTED.

[quaff]

I admit your title "Propagation.REQUIRES_NEW May cause all connections to be occupied".
application should deal REQUIRES_NEW properly, there is no bug or improve space here, at most adding note in docs. Do you agree?

[wlizhi]

Thank you for taking the time to listen to me describe the problem,That's what I mean. If we must use 'REQUIRES_NEW' needs to be dealt with properly. It would be great if it could be mentioned in the documentation to make other developers aware of it.

[quaff]

Thank you for taking the time to listen to me describe the problem,That's what I mean. If we must use 'REQUIRES_NEW' needs to be dealt with properly. It would be great if it could be mentioned in the documentation to make other developers aware of it.

The javadoc and reference should mention that REQUIRES_NEW will retain double resources(such as JDBC connections) if a current transaction already exists, WDYT @jhoeller ?

[roma2341]

Requires_new is king of side effects. When i began to wrote integration tests i understood that this propagation prevents my transactions from rollback, but i didnt expect this behaviour would make my database dirty after a test, i had transactional annotation on my test methods and i know that requires_new starts new transaction so it wouldn't be rolled-back, only outer transaction would be rolled-back. And i don't know how to avoid this behaviour in my tests, only way is to not to use requires_new. And what is weird i have never heard about this problem when read articles about testing spring applications. And why i also dont like requires_new is that it can make my program throw pessimistic lock exceptions and it's very hard to find a reason