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

Doc: @Transactional.isolation does not guarantee the specified isolation level [SPR-16463] #21008

Closed
spring-projects-issues opened this issue Feb 3, 2018 · 5 comments

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Feb 3, 2018

Valentin Kovalenko opened SPR-16463 and commented

Let's imagine two bean methods:

@Transactional(propagation = Propagation.REQUIRED, isolation = Isolation.READ_UNCOMMITTED)
public void methodA(Runnable action) {
  jdbc.execute("select 'inside methodA before methodB'");
  action.run();
  jdbc.execute("select 'inside methodA after methodB'");
}
@Transactional(propagation = Propagation.REQUIRED, isolation = Isolation.SERIALIZABLE)
public void methodB() {
  jdbc.execute("select 'inside methodB'");
}

and we want to use them like this:

public void use() {
  bean.methodA(bean::methodB);
}

This is a tricky case: it can't be implemented without violating the semantics of either Transactional.propagation or Transactional.isolation, because modern RDBMS do not allow changing an isolation level in the middle of a transaction, and Spring does not even try doing this. A user, however, may not realize this fact, or may not realize the fact that he actually has the situation bean.methodA(bean::methodB) in his project. Thus the user may believe that methodB is always executed in a transaction context with Isolation.SERIALIZABLE, which does not happen to be true.

The same is true not only for Propagation.REQUIRED, but also for Propagation.MANDATORY. This means that annotating a method with @Transactional(propagation = Propagation.MANDATORY, isolation = Isolation.SERIALIZABLE) does not guarantee that the method will be executed in a transaction context with Isolation.SERIALIZABLE.

I think that such behaviour violates the principle of least astonishment and may easily lead to bugs related to a usage of an incorrect isolation level. However, it may not be a good idea to always throw an exception when Spring cannot guarantee the demanded level of isolation. Possibly this can be solved by throwing an exception by default but providing a way to disable it by explicitly specifying which semantics the Spring framework should violate in such situations: Transactional.propagation or Transactional.isolation. I believe the solution definitely requires discussing.

The complete description of this situation can be found here: https://sites.google.com/site/aboutmale/techblog/transactionalcatch


Affects: 4.3.14

Issue Links:

  • #8870 Add warning and/or exception facility to propagating transactions that try to change the isolation level

Referenced from: commits cc77b4b, 0ac117f, f789895

Backported to: 4.3.15

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 5, 2018

Juergen Hoeller commented

This is essentially by design: Just like a transaction timeout and the read-only flag, an isolation level is 'just' a characteristic to be honored for newly started transactions. For participating in an existing transaction, it is the responsibility of the caller to semantically align there, and Spring does not give any particular guarantees about the validity of the overall arrangement.

Point taken that we could have clearer documentation notes about this; I'll repurpose this ticket for it. I'll specifically hint at the existing "validateExistingTransaction" flag on our transaction manager implementations which checks the isolation level and read-only status of an existing transaction before participating, as a kind of assertion in order to provide accidental mismatches.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 5, 2018

Valentin Kovalenko commented

@juergen.hoeller Oh, I see, thanks! AbstractPlatformTransactionManager.setValidateExistingTransaction is actually a good solution to the problem. Unfortunately, I didn't know that this flag exists. Since it is impossible to change the default value of this flag to true without breaking backwards compatibility, I agree that it would be nice to at least have it mentioned in the @Transactional and in the TransactionTemplate.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 5, 2018

Valentin Kovalenko commented

@juergen.hoeller I just have tried AbstractPlatformTransactionManager.setValidateExistingTransaction and found out that Spring validates current isolation level naively :

if (currentIsolationLevel == null || currentIsolationLevel != definition.getIsolationLevel()) {
  Constants isoConstants = DefaultTransactionDefinition.constants;
  throw new IllegalTransactionStateException("Participating transaction with definition [" +
      definition + "] specifies isolation level which is incompatible with existing transaction: " +
      (currentIsolationLevel != null ?
          isoConstants.toCode(currentIsolationLevel, DefaultTransactionDefinition.PREFIX_ISOLATION) :
          "(unknown)"));
}

and hence throws

org.springframework.transaction.IllegalTransactionStateException:
Participating transaction with definition [PROPAGATION_MANDATORY,ISOLATION_READ_UNCOMMITTED; 'rwTxManager',-java.lang.Throwable] specifies isolation level which is incompatible with existing transaction: ISOLATION_READ_COMMITTED

while it is obviously perfectly fine to execute method requiring ISOLATION_READ_UNCOMMITTED in a transaction context with ISOLATION_READ_COMMITTED, because the latter isolation level specifies stronger consistency guarantees than the former one. I think that Spring should use comparison accounting actual semantics of different isolation levels:

TRANSACTION_READ_UNCOMMITTED < TRANSACTION_READ_COMMITTED < TRANSACTION_REPEATABLE_READ < TRANSACTION_SERIALIZABLE
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 5, 2018

Juergen Hoeller commented

This was done on purpose back in #8870 (see the comments). The isolation level setting on a transaction definition is not a minimum isolation requirement, it is the actual isolation level desired. An inner transaction might not just be able to execute in READ_UNCOMMITTED, it might explicitly want READ_UNCOMMITTED for its non-locking semantics. Running under a stronger isolation level does effectively change the semantic context that the inner transaction executes in: Some sequences of operations might not even be able to complete with a stronger locking model at runtime. This is why we chose strict compliance for validateExistingTransaction, and I would argue that this is fine for such a strict validation mode. If you want lenient handling, you can always opt to not set that flag in the first place.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 5, 2018

Valentin Kovalenko commented

An inner transaction might not just be able to execute in READ_UNCOMMITTED, it might explicitly want READ_UNCOMMITTED for its non-locking semantics.
You are right. I was thinking only about consistency and forgot that this is not the whole story. Isolation level may indeed affect locking and hence may change the behaviour of a program.

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.