Make JpaVendorAdapters JTA-aware (in particular for Hibernate 5.1/5.2) [SPR-16162]

[spring-projects-issues]

Alexandru-Constantin Bledea opened SPR-16162 and commented

One case that I keep coming across every once in a while is the issue that was described in #19524.

So I basically want to reopen that case but with an improvement suggestion rather than a bug report.

The problem as described there was that "The underlying technical problem is that we can't influence Hibernate's connection release mode depending on the transaction setup: simply because JPA doesn't allow us to reliably introspect the transaction setup in general."

I've dug a bit into this and I'm wondering why we need to know the transaction manager at the point of creating the JpaVendorAdapter? The JpaVendorAdapter is only used for the EntityManagerFactory and when we are building the EntityManagerFactory we do know what transaction type we will be using. This information is available from the JPA PersistenceUnitInfo.

I'm thinking if it would make sense to add an additional method to JpaVendorAdapter, something like, getAdditionalJpaPropertiesMapByTransactionType() that is called after the default getJpaPropertiesMap method. That way we can selectively enrich the properties of one transaction type or the other.

By separating the properties by transaction type, it would be possible to 'autoconfigure' the backwards compatibility with spring in standalone mode and also keep the backwards compatibility with the JTA configuration.

What do you think?

---

Affects: 4.3.6

Issue Links:

• Doc: HibernateJpaVendorAdapter's "prepareConnection" may interfere with Hibernate 5.1+ in a JTA environment [SPR-14957] #19524 Doc: HibernateJpaVendorAdapter's "prepareConnection" may interfere with Hibernate 5.1+ in a JTA environment

Referenced from: pull request #1591

Backported to: 4.3.13

[spring-projects-issues]

Juergen Hoeller commented

Good point! Let's see what we can do there.

[spring-projects-issues]

Alexandru-Constantin Bledea commented

Pull request here
#1591

If the transaction type cannot be determined for whatever reason, we still override the connection release mode just to maintain the previous behavior, but I tested this with WebLogic and it identifies the JTA and all is good.

To be honest, I'm wondering if it still makes sense to have two methods for the jpa properties. They're only used one after the other anyway, I'm thinking of merging them back and just adding the TransactionType parameter to the original method, what do you think?

[spring-projects-issues]

Juergen Hoeller commented

Alright, this looks like a good start! I intend to approach the subclass/superclass interaction a bit differently, and indeed, we should merge the JPA properties access to a single method which takes the transaction type or maybe even the entire PersistenceUnitInfo, declared as a default method that calls the standard JpaVendorAdapter.getJpaPropertyMap() variant. In any case, it's great to know that the changes in the PR work for your scenario!

So I guess this back on the 5.0.2 roadmap for mid next week now :-) For the sake of timing, I'll spend some time on it right away this afternoon, inspired by your pull request. Just wondering: Can you consume that revision in 5.0.2? Or are you strictly bound to 4.3.x at this point?

[spring-projects-issues]

Alexandru-Constantin Bledea commented

I performed the changes on 5.0.2 but it would be great if once this is implemented it would get backported to 4.3.x.
The reason being that spring 5.x requires java8 and the weblogic version in our organization is 12.1.x which doesn't quite work with spring 5.x unfortunately.

I am quite happy to work with you to get this thing rolling, Just let me know how I can help and I will gladly do it.
I'm not sure if we can pass PersistenceUnitInfo to the AbstractEntityManagerFactoryBean because the LocalEntityManagerFactoryBean doesn't even have the PersistenceUnitInfo (just like we can't determine the transaction type for it just yet).

Just let me know how you want to proceed and I can try to support you the best that I can.

[spring-projects-issues]

Juergen Hoeller commented

I've prepared a pass for master: JpaVendorAdapter has an overloaded getJpaPropertyMap(PersistenceUnitInfo) method now which is being called when a PersistenceUnitInfo is available; otherwise plain getJpaPropertyMap() gets called. AbstractEntityManagerFactoryBean checks its existing getPersistenceUnitInfo() method to make that decision, and only LocalContainerEntityManagerFactoryBean happens to override it.

Since we can't declare getJpaPropertyMap(PersistenceUnitInfo) as a default method in the JpaVendorAdapter interface for the 4.3.x line, I've also added a variant to AbstractJpaVendorAdapter which can be called through a downcast. That part is what I intend to backport to 4.3.13.

[spring-projects-issues]

Alexandru-Constantin Bledea commented

Sounds good. Also I think that the javadoc for prepareConnection should be updated to reflect the fact that now if Spring is able to determine that we're using JTA it backs off from overriding the connection release configuration which causes hibernate to pick what it considers the best strategy.

[spring-projects-issues]

Juergen Hoeller commented

Good catch about the javadoc; revised now.

A minor revision of the implementation approach above: Due to package interdependencies, downcasting to AbstractJpaVendorAdapter isn't really viable. So for 4.3.13, I'll add the same method to the JpaVendorAdapter instead, just not as a default method, and AbstractEntityManagerFactoryBean will simply call it with a catch (AbstractMethodError) block for third-party adapters not implementing that method.

[spring-projects-issues]

Alexandru-Constantin Bledea commented

I tested the implementation in master and it skips configuring JTA for now.

One thing I noticed is that now, if the transaction type is unknown, we are no longer overriding the default properties, was this intended?

Also, I took a look into LocalEntityManagerFactory and I'm thinking that we could also try to get its persistence unit info to detect the transaction type with something like

@Nullable
@Override
public PersistenceUnitInfo getPersistenceUnitInfo() {
     DefaultPersistenceUnitManager pum = new DefaultPersistenceUnitManager();
     pum.afterPropertiesSet();
     return pum.obtainPersistenceUnitInfo(getPersistenceUnitName());
}

Admittedly, I'm not sure if this persistence unit info will be 100% accurate because internally the PersistenceProvider has its own way of interpreting the persistence.xml file but at least for the transaction type (which is really the only thing that we care about at this point) should be accurate.

What do you think?

[spring-projects-issues]

Juergen Hoeller commented

Oops, I indeed missed the LocalEntityManagerFactoryBean case where we should also enforce the on-close connection release mode. I'll refactor the HibernateJpaVendorAdapter arrangement to set the properties through a delegate which just leaves the connection-preparation decision up to the corresponding getJpaPropertyMap(PersistenceUnitInfo) vs getJpaPropertyMap() entry point.

Fortunately, LocalEntityManagerFactoryBean as the simple "Java SE" bootstrapping mechanism per the JPA spec only works with local transactions to begin with. So it is a safe assumption to make there that we can set the on-close connection release mode without extra checks. It's only the "Java EE" bootstrapping variant in LocalContainerEntityManagerFactoryBean where we have to take JTA into account, and in particular JTA as in application server environments with managed connection pools and strict connection borrowing.

Having the mechanism generically extended to react to the PersistenceUnitInfo at the JpaVendorAdapter level seems a good choice in any case. There might be other properties that should only be set in certain deployment scenarios, potentially in third-party adapters or - not unusual either - in custom subclasses of our HibernateJpaVendorAdapter adapter.

[spring-projects-issues]

Juergen Hoeller commented

HibernateJpaVendorAdapter is updated in master now, as per the discussion above. I've also added a corresponding note to the JpaVendorAdapter javadoc.

[spring-projects-issues]

Alexandru-Constantin Bledea commented

I took a quick look over the backport of the improvement and i think that line 175 should be changed to use the connectionReleaseOnClose parameter
rather than the property from the jpa dialect.

spring-framework/spring-orm/src/main/java/org/springframework/orm/jpa/vendor/HibernateJpaVendorAdapter.java

Line 175 in d52d9fd

if (this.jpaDialect.prepareConnection) {

Update: I was looking at an older version, looks great in 4.3.x. Thank you!

[spring-projects-issues]

Juergen Hoeller commented

Actually, your point seems valid: I accidentally skipped that line when merging. I've revised this in 4.3.x, so that check is in sync with master now.

[spring-projects-issues]

Alexandru-Constantin Bledea commented

Hibernate specifies that AFTER_TRANSACTION release mechanism should never be used for JTA transactions in the case of application server managed datasource
https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/ConnectionReleaseMode.java#L31

But the hibernate jta vendor adapter suggests that mode as a posibility in such cases. I'm wondering the suggestion should be removed to not get inconsistent behavior.
https://github.com/spring-projects/spring-framework/blob/master/spring-orm/src/main/java/org/springframework/orm/jpa/vendor/HibernateJpaVendorAdapter.java#L94

[spring-projects-issues]

Juergen Hoeller commented

To the best of my experience, AFTER_TRANSACTION works fine in most JTA scenarios. It's just that some DataSource implementations might create a log entry about a long-held connection... On the other hand, WebSphere classic was known to return a new connection for every access attempt during a JTA transaction, so reusing the application-level connection handle for the entire transaction - ON_CLOSE or at least AFTER_TRANSACTION - can be an important optimization.

All in all, Hibernate's javadoc isn't as nuanced as it should be there. If broader reuse of connection handles works in your app server scenario, there's nothing wrong with it; it's generally recommendable to go with the broadest mode possible. You'll find out very quickly whether it works anyway - the app server will reject access to your connection if you're misusing it according to the platform's policies - and can easily downgrade to AFTER_STATEMENT if necessary. Since our comment is just in javadoc there, it's really just a hint to try both modes if you're in the process of fine-tuning your setup.

[spring-projects-issues]

Alexandru-Constantin Bledea commented

Good to know, I have read some articles suggested that AFTER_TRANSACTION is faster but I saw that hibernate suggests otherwise so I wasn't sure what the best approach is. I will try AFTER_TRANSACTION myself to see if I will notice some inconsistent behavior. Thanks.