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

EclipseLinkJpaDialect applies per-transaction isolation level to shared DatabasePlatform #24802

Open
deanwong opened this issue Mar 27, 2020 · 14 comments
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement

Comments

@deanwong
Copy link

deanwong commented Mar 27, 2020

spring orm version: 5.1.4.RELEASE

In EclipseLinkJpaDialect class line 81

if (definition.getIsolationLevel() != TransactionDefinition.ISOLATION_DEFAULT) {
	// Pass custom isolation level on to EclipseLink's DatabaseLogin configuration
	// (since Spring 4.1.2)
	UnitOfWork uow = entityManager.unwrap(UnitOfWork.class);
	uow.getLogin().setTransactionIsolation(definition.getIsolationLevel());
}

for instance:
If txA beginTransaction and there is Isolation.SERIALIZABLE around the repository class, the TransactionIsolation in uow.getLogin() will be changed to Isolation.SERIALIZABLE.
Then txB beginTransaction with Isolation.DEFAULT, the condition is not met, TransactionIsolation in uow.getLogin() still be SERIALIZABLE.

I think the right code should like this:

if (definition.getIsolationLevel() != TransactionDefinition.ISOLATION_DEFAULT
   && definition.getIsolationLevel() != entityManager.unwrap(UnitOfWork.class).getLogin().setTransactionIsolation()) {
	// ... 
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 27, 2020
@jhoeller jhoeller self-assigned this Mar 27, 2020
@jhoeller jhoeller added in: data Issues in data modules (jdbc, orm, oxm, tx) type: bug A general bug labels Mar 27, 2020
@sbrannen
Copy link
Member

@deanwong, I think you meant to use getTransactionIsolation() instead of setTransactionIsolation() -- right?

@deanwong
Copy link
Author

deanwong commented Mar 29, 2020

@sbrannen yes

correct

definition.getIsolationLevel() != entityManager.unwrap(UnitOfWork.class).getLogin().getTransactionIsolation()

@jhoeller
Copy link
Contributor

UnitOfWork is supposed to be an independent instance for every EntityManager, so I wonder why there would be old serialization settings exposed there? Every fresh UnitOfWork within a new transaction should come with EclipseLink's default settings... We just override those if necessary, and at the end of every transaction the UnitOfWork and its settings gets closed and disposed.

@deanwong
Copy link
Author

We use Hikari as connection pool, does that affect the behavior?

@jhoeller
Copy link
Contributor

I suspect that EclipseLink applies the transaction isolation level to the underlying JDBC Connection but does not restore the original isolation level there before it returns the Connection to the pool. As far as I can see, it does not expose the current connection isolation level via DatabaseLogin.getTransactionIsolation() though, so the suggested check above won't help. We'd rather have to make sure to restore the original JDBC isolation level ourselves, similar to how we do it with Hibernate. A bit of a shame since EclipseLink has a higher-level API for this, in contrast to Hibernate, so it seems a bit awkward to manually dive down to JDBC level for restoring.

@deanwong
Copy link
Author

deanwong commented Mar 31, 2020

@jhoeller
I just create a minimize project to reproduce this problem. Hope that could help.
There are 4 interactions with DB, second query surrounding with SERIALIZABLE, and the following queries all use this isolation level.

@toalexivanov
Copy link

transactionIsolation field in
org.eclipse.persistence.internal.databaseaccess.DatabasePlatform is not
synchronized. This is the root cause.

DatabasePlatform stores various database settings that rarely
change at runtime. Perhaps that is what implementors of EclipseLink meant for
transactionIsolation as well, set it once and forget. This is how most
database applications use it anyway. It is rare for an application to use
transaction isolation level other than the default.

An instance of this class is called by JpaTransactionManager which is a Spring
bean in our application. Application accesses this bean from multiple threads.
Now you can imagine multiple service threads starting transactions with
different isolation levels concurrently. Also statement in
EclipseLinkAdapter.beginTransaction() checks requested transaction isolation
level and simply returns without setting it if it is equal to a default value.
Transaction isolation level is set to default automatically in Spring if it is
not set explicitly. In majority of our usages, we don't set this level explicitly
for one simple reason - default isolation level READ_COMMITTED is sufficient
for our purposes most of the time. However it means that transaction by explicitly
setting non-default isolation level overrides this level for all subsequent transactions.
Not only that, depending on timing, a transaction in one thread could silently
override transaction isolation level in another thread.

To remedy this problem I supplied CustomOraclePlatform bean where I override
setTransactionIsolation() and getTransactionIsolation(). These methods make use
of a ThreadLocal field represeting isolation level. Now each thread gets its own
isolation level. However transactions with different isolation levels
in the same thread would still experience the problem. Also cached thread pools
do not reset ThreadLocals. For that we need to prevent EclipseLinkJpaDialect
from invoking short-circuit check for default level. This can be
be done by translating TransactionDefinition.ISOLATION_DEFAULT to
TransactionDefinition.ISOLATION_READ_COMMITTED (or whatever default level is
appropriate for your database) in EclipseLinkJpaDialect.beginTransaction(). This
is exactly what I did in CustomEclipseLinkJpaDialect.

@jhoeller
Copy link
Contributor

Well spotted, these calls internally delegate to the shared DatabasePlatform instance... despite appearing on an independent UnitOfWork instance and its DatabaseLogin. This means that EclipseLink's own support for transaction isolation levels is effectively unusable for our purposes :-( We'll see whether there is some way to apply an independent isolation level without modifying the DatabasePlatform implementation; otherwise we'll have to document this as being severely limited.

@jhoeller jhoeller removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 16, 2020
@jhoeller jhoeller added this to the 5.2.8 milestone Jun 16, 2020
@jhoeller jhoeller changed the title spring-orm tx Isolation bug in eclipseLink EclipseLinkJpaDialect applies per-transaction isolation level to shared DatabasePlatform Jun 16, 2020
@jhoeller
Copy link
Contributor

I've locally applied a different patch: Using a custom TransactionalDatabaseLogin class for the UnitOfWork which locally stores the isolation level and only falls back to the platform-defined isolation level if no transaction-specific isolation level has been set. This seems to work fine so far, and decouples the solution from the vendor-specific DatabasePlatform implementation. It also covers the default-isolation-level problem (when mixing transactions with a custom isolation level next to transactions without a specific isolation level) as far as it will set the platform-defined transaction isolation by default, so we don't need to reset the isolation level ourselves.

@jhoeller
Copy link
Contributor

Quick update: There seems to be a problem with propagating such a custom login instance to the actual session used. EclipseLink's internal architecture doesn't make this very obvious... let's see whether we can reach a solution here, otherwise we might have to go with a different approach.

@jhoeller
Copy link
Contributor

It looks like this can only really be solved through a custom DatabasePlatform subclass for your particular dialect, overriding set/getTransactionIsolation to take the Spring transaction isolation level into account in a thread-local fashion, in interaction with a custom variant of EclipseLinkJpaDialect. To some degree I wonder whether we should revert to rejecting custom isolation levels by default in EclipseLinkJpaDialect... but then this would also prevent single-threaded scenarios and such custom DatabasePlatform variants. Maybe there's something we can do to make the latter easier to implement, and for the standard case we'll document the limitations.

@toalexivanov
Copy link

Can you just set transaction isolation level directly on a JDBC Connection thus overriding whatever EclipseLink does?

@jhoeller
Copy link
Contributor

We could try to operate directly on the JDBC Connection but we'd have to clarify when exactly an EclipseLink Session obtains its JDBC Connection and when exactly it returns the Connection to the pool. Hibernate has well-defined connection policies for that purpose, whereas with EclipseLink we'd probably have to rely on implementation behavior (hoping that it won't change).

@jhoeller
Copy link
Contributor

jhoeller commented Jun 4, 2023

We're rolling an EclipseLinkJpaDialect refinement into 6.0.10 and 5.3.28 now: see #29997 (comment)

While this still operates on a shared DatabasePlatform instance, it should work reliably in concurrent scenarios as well as in scenarios with inconsistent transaction isolation definitions now. This seems to be the best we can do with current EclipseLink, even in its 3.0 and 4.0 version.

@jhoeller jhoeller modified the milestones: 6.x Backlog, General Backlog Jul 12, 2023
@jhoeller jhoeller removed their assignment Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants