Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TransactionTimedOutException during DefaultLockRepository.acquire() #3307

Closed
astrubel opened this issue Jun 12, 2020 · 10 comments
Closed

TransactionTimedOutException during DefaultLockRepository.acquire() #3307

astrubel opened this issue Jun 12, 2020 · 10 comments

Comments

@astrubel
Copy link
Contributor

Hi,
I'm facing to TransactionTimedOutException that occurs at DefaultLockRepository.acquire(). I think is the parameter timeout = 1 on the @Transactional annotation on it.
Why the lock is not retried in this case ? It's a naive question, really.
Thank you in advance!

@astrubel astrubel added status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Jun 12, 2020
@artembilan
Copy link
Member

I think you are right: we need to add a TransactionTimedOutException into this catch:

catch (TransientDataAccessException e) {
					// try again
}

@artembilan artembilan added in: jdbc backport 5.2.x and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Jun 12, 2020
@artembilan artembilan added this to the 5.4 M1 milestone Jun 12, 2020
@astrubel
Copy link
Contributor Author

I will create a pull request for it.
Could you backport to 4.3.x too ?

@artembilan
Copy link
Member

Could you backport to 4.3.x too ?

Marked respectively.
Will backport after the Pull Request.

Thanks

@astrubel
Copy link
Contributor Author

What does represent the parameter timeout on the @Transaction annotation? Is it contains the access time to the database or only the transaction opening and processing time?

@artembilan
Copy link
Member

Not sure what is your concern on the matter, but see its JavaDocs:

/**
	 * The timeout for this transaction (in seconds).
	 * <p>Defaults to the default timeout of the underlying transaction system.
	 * <p>Exclusively designed for use with {@link Propagation#REQUIRED} or
	 * {@link Propagation#REQUIRES_NEW} since it only applies to newly started
	 * transactions.
	 * @return the timeout in seconds
	 * @see org.springframework.transaction.interceptor.TransactionAttribute#getTimeout()
	 */
	int timeout() default TransactionDefinition.TIMEOUT_DEFAULT;

So, therefore it a timeout for the whole call including Java and DB access.

@astrubel
Copy link
Contributor Author

So I think that 1s is not enough for db access through particular networks.

How can we expose this parameter to setup into project side?

Is it possible to replace @Transactional annotation by TransactionTemplate into DefaultLockRepository.acquire() and setup TransactionTemplate.timeout?

@Override
public boolean acquire(String lock) {
   // save previous transactionTemplate values
   int prevIso = transactionTemplate.getIsolationLevel();
   int prevTimeout = transactionTemplate.getTimeout();

   // setup acquire transaction
   transactionTemplate.setIsolationLevel(Isolation.SERIALIZABLE);
   transactionTemplate.setTimeout(this.getTimeout());
   transactionTemplate.execute(st -> {
      deleteExpired(lock);
      if (this.template.update(this.updateQuery, new Date(), this.region, lock, this.id) > 0) {
         return true;
      }
      try {
         return this.template.update(this.insertQuery, this.region, lock, this.id, new Date()) > 0;
      }
      catch (DuplicateKeyException e) {
         return false;
      }
   });

   // restore previous transactionTemplate values
   transactionTemplate.setIsolationLevel(prevIso);
   transactionTemplate.setTimeout(prevTimeout);
}

@artembilan
Copy link
Member

I think the best and simplest way is to extend a DefaultLockRepository and override its acquire() with your own @Transactional.
But I see your point and really tended to remove that timeout altogether. No need to be so restrictive. Let's just rely on the underlying transaction management as it is explained on that timeout() option!

Feel free to do such a change in your current PR - and we'll review & merger it shortly.

Thanks

@astrubel
Copy link
Contributor Author

The current PR concern the management of the timeout exception. So I don't need to modify this one.

You are true, for a particular case the simplest way is to extend and override!

Thank you so much for this talk ;)

astrubel pushed a commit to astrubel/spring-integration that referenced this issue Jun 16, 2020
artembilan pushed a commit that referenced this issue Jun 16, 2020
Fixes #3307

The `DefaultLockRepository.acquire()` is transactional method and
can fail with the `TransactionTimedOutException`.
When we call `JdbcLock.lock()`, we expect an attempt until we really obtain a lock.

* Treat a `TransactionTimedOutException` as a `TransientDataAccessException` and
therefore retry a locking attempt logic

**Cherry-pick to 5.3.x, 5.2.x & 4.3.x**

(cherry picked from commit b0cd015)
artembilan pushed a commit that referenced this issue Jun 16, 2020
Fixes #3307

The `DefaultLockRepository.acquire()` is transactional method and
can fail with the `TransactionTimedOutException`.
When we call `JdbcLock.lock()`, we expect an attempt until we really obtain a lock.

* Treat a `TransactionTimedOutException` as a `TransientDataAccessException` and
therefore retry a locking attempt logic

**Cherry-pick to 5.3.x, 5.2.x & 4.3.x**

(cherry picked from commit b0cd015)
artembilan added a commit that referenced this issue Jun 16, 2020
Fixes #3307

The `DefaultLockRepository.acquire()` is transactional method and
can fail with the `TransactionTimedOutException`.
When we call `JdbcLock.lock()`, we expect an attempt until we really obtain a lock.

* Treat a `TransactionTimedOutException` as a `TransientDataAccessException` and
therefore retry a locking attempt logic

**Cherry-pick to 5.3.x, 5.2.x & 4.3.x**

(cherry picked from commit b0cd015)

# Conflicts:
#	spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/lock/JdbcLockRegistry.java
@artembilan
Copy link
Member

Cherry-picked to 5.3.x & 5.2.x.
Back-port to 4.3.x as b4d9831

@astrubel ,

Thank you for contribution; looking forward for more!

artembilan added a commit that referenced this issue Jun 16, 2020
@astrubel
Copy link
Contributor Author

@artembilan Thank you very much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants