DefaultLockRepository can misbehave when DB transaction is already active

[xak2000]

Currently acquire method of DefaultLockRepository has @Transactional(isolation = Isolation.SERIALIZABLE) annotation.

spring-integration/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/lock/DefaultLockRepository.java

Line 162 in a843dae

@Transactional(isolation = Isolation.SERIALIZABLE)

But I wander what will be if I try to acquire a LOCK from the method that is already annotated with @Transactional?

If the transaction is already started, the default propagation level is REQUIRED and that does mean that already opened transaction will be supported instead of creating a new one. And the documentation of @Transactional.isolation attribute explicitly says:

Exclusively designed for use with Propagation.REQUIRED or Propagation.REQUIRES_NEW since it only applies to newly started transactions.

I'm not sure what does it mean in the context of REQUIRED propagation when it's an inner REQUIRED transaction, but it looks like it will just do nothing with the isolation level if the transaction is already started. So the isolation level will be the same that is set by most outer transaction (real DB transaction that is already started).

To me it looks like @Transactional(isolation = Isolation.SERIALIZABLE) annotation must include propagation = Propagation.REQUIRES_NEW attribute to make sure that LOCK acquiring will always be done in a separate transaction with SERIALIZABLE isolation level.

I also see other potential problems with the shared transaction between user's code and LOCK repository. For example, any UPDATE of a record in INT_LOCK table will be blocked if other thread already acquired the lock and now executes the user's code under this LOCK (in the same transaction that acquired the lock). In this case an EXCLUSIVE DB LOCK on an INT_LOCK's record will be hold by a first transaction for the whole time while user's (outer) transaction is not committed or rolled back. So, e.g. JdbcLockRegistry.JdbcLock.tryLock method will be blocked for a potentially long time.

Another potential problem could be for the retry logic in the case of transient DB exception (e.g. deadlock).
Currently JdbcLockRegistry.JdbcLock.lock() method does retry is the case of one of:

• TransientDataAccessException
• TransactionTimedOutException
• TransactionSystemException

But this retry logic could not always work correctly if it will not start a new DB transaction before each retry. With current implementation of acquire() method it looks like it's not a problem, but if the method will e.g. SELECT before UPDATE in a REPEATABLE_READ transaction, the logic could be flawed because SELECT will always return the same result that was returned when it was executed the 1st time in this transaction. Anyway, it looks like the retry logic in JdbcLockRegistry.JdbcLock was implemented with the assumption that a new transaction will be started by each acquire method execution.

Another problem with a shared transaction I could imagine: a user's transaction that acquires and releases the LOCK and then executes a method with REQUIRES_NEW transaction that tries to acquire the same (already released) LOCK. It will not be able to do this because of a transaction opened by the caller method that is still active and just suspended. The java LOCK is already released by the caller's method, but the DB EXCLUSIVE LOCK on the deleted DB record of INT_LOCK table is still held by this suspended transaction, so new transaction in the called method could not UPDATE or INSERT the same record into the table:

public class Service1 {
  @Transactional
  public void method1() {
    Lock lock = lockRegistry.obtain("lock1");
    lock.lock();
    try {
      // some logic
    } finally {
      lock.unlock();
    }
    
    service2.method2(); // current transaction is suspended but is still active
  }
}

public class Service2 {
  @Transactional(propagation = Propagation.REQUIRES_NEW)
  public void method2() {
    Lock lock = lockRegistry.obtain("lock1");
    lock.lock(); // it will never succeed because DB X LOCK is still held by another transaction!
    // JdbcLockRegistry.JdbcLock.lock() will try again indefinitely because MySQL exception is:
    // SQL Error [1205] [40001]: Lock wait timeout exceeded; try restarting transaction
    // So I suppose it will be TransientDataAccessException or TransactionTimedOutException
    // which is trigger for "try again" to execute DefaultLockRepository.acquire().
  }
}

Also reusing the same transaction opened by user's code could increase the probability of deadlocks, as user's transaction could already hold some DB locks at the point of acquiring the LOCK, and if it does this in multiple places in reverse order, a deadlock will happen.

All in all, to me it looks like DefaultLockRepository and JdbcLockRegistry were designed with the assumption that a new transaction will be opened on each call to any repository method instead of supporting an already opened transaction. Am I right?

A also noticed the @Transactional annotation on the whole DefaultLockRepository class that looks strange to me as some methods like setRegion doesn't require a transaction. The annotation on the class is also applies to all methods of subclasses, so this makes extending of DefaultLockRepository class harder. This is not a major problem, though. But propagation = Propagation.REQUIRES_NEW attribute also should be added here (or @Transactional annotation should be moved from the class level to appropriate methods level).

[artembilan]

Hi @xak2000 !

I think all your investigations and conclusions are valid.

I guess it is was designed in mind when this one is used outside of any end-user transactions.
Therefore its explicit usage of that @Transactional to ensure that we do locking in the TX scope.

Would you mind to provide a PR with the fix you have in mind and we will continue a discussion over there (if any).

Thank you!

[xak2000]

Hi @artembilan !

I think adding propagation = Propagation.REQUIRES_NEW to the @Transactional on the class is enough. Or maybe the annotation could be removed from the class level altogether and explicitly added to each method, that does any SQL queries. What do you think will be better?

I'm not sure though how attributes of this annotation are merged when the annotation is present on both the class level and the method level.

So, if the class has

@Transactional(propagation = Propagation.REQUIRES_NEW)

and the method has

@Transactional(isolation = Isolation.SERIALIZABLE)

does it mean that the method will also effectively have propagation = Propagation.REQUIRES_NEW attribute inherited from a class level annotation? Or it will have the default propagation that is REQUIRED?

So, the effective method-level annotation will be:

@Transactional(propagation = Propagation.REQUIRES_NEW, isolation = Isolation.SERIALIZABLE)

or

@Transactional(propagation = Propagation.REQUIRED, isolation = Isolation.SERIALIZABLE)

?

[artembilan]

If you are interested in that logic, then it is here in the AbstractFallbackTransactionAttributeSource:

protected TransactionAttribute computeTransactionAttribute(Method method, @Nullable Class<?> targetClass) {
		// Don't allow no-public methods as required.
		if (allowPublicMethodsOnly() && !Modifier.isPublic(method.getModifiers())) {
			return null;
		}

		// The method may be on an interface, but we need attributes from the target class.
		// If the target class is null, the method will be unchanged.
		Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass);

		// First try is the method in the target class.
		TransactionAttribute txAttr = findTransactionAttribute(specificMethod);
		if (txAttr != null) {
			return txAttr;
		}

		// Second try is the transaction attribute on the target class.
		txAttr = findTransactionAttribute(specificMethod.getDeclaringClass());
		if (txAttr != null && ClassUtils.isUserLevelMethod(method)) {
			return txAttr;
		}

		if (specificMethod != method) {
			// Fallback is to look at the original method.
			txAttr = findTransactionAttribute(method);
			if (txAttr != null) {
				return txAttr;
			}
			// Last fallback is the class of the original method.
			txAttr = findTransactionAttribute(method.getDeclaringClass());
			if (txAttr != null && ClassUtils.isUserLevelMethod(method)) {
				return txAttr;
			}
		}

		return null;
	}

As you see there is no merging for those class-level and method-side annotations: if method has an annotation, it wins. Otherwise it is taken from the class-level if exists.

I think you are right with your logic about class-level for this DefaultLockRepository: let's just remove it from the class-level, and have it fine-grained really on the specific method which does some data manipulations.

[xak2000]

@artembilan Thank you for mentioning of AbstractFallbackTransactionAttributeSource. It's really easy when you know where to look! :)

I created a PR #3684. Please take a look when you have time.