Re: [hibernate-dev] Usage of Session#getTransaction() being banned from the Hibernate Search code

On 14 September 2016 at 17:16, Steve Ebersole <steve(a)hibernate.org&gt; wrote: > To be clear, I mean "never end the transaction locally". It's like any > resource handling... if you start/begin/open something you should > stop/end/close it. IMHO. Thanks for the clarifications. I agree on the "who opens close" principle.. I probably shouldn't be doing the commit then, if I skip the begin? > > > On Wed, Sep 14, 2016 at 11:15 AM Steve Ebersole <steve(a)hibernate.org&gt; wrote: >> >> Yes, it was intentional. As you say it is totally reasonable. >> >> The problem is "matching". Like it was no problem to call begin() before >> but what happened if the txn was commited multiple times was wonky. In fact >> JPA explicitly forbids it multiple calls to commit (as well as multiple >> calls to begin). >> >> I personally think code like this is just awful: >> >> Session s = ...; >> s.accessTransaction().begin(); >> // do some stuff >> // but never end the txn >> >> >> >> On Wed, Sep 14, 2016 at 10:59 AM Sanne Grinovero <sanne(a)hibernate.org&gt; >> wrote: >>> >>> Hi Steve, >>> as a follow up of migrating #getTransaction() usage to >>> session.accessTransaction() I now noticed a difference: >>> >>> we had previous code like this: >>> >>> Transaction transaction = session.getTransaction(); >>> transaction.begin(); >>> >>> Which worked fine even though the user is actually using an >>> EntityManager (the Hibernate Search integration code consistently uses >>> the underlying Session so that it works in either case..). >>> It also worked fine in both JTA and other transaction modes; in case >>> of JTA w'd previously have started the transaction on the >>> TransactionManager (as well). >>> >>> But the new code now requires a bit more care: >>> >>> Transaction transaction = session.accessTransaction(); >>> if ( transaction.isActive() == false ) { >>> transaction.begin(); >>> } >>> >>> as otherwise the transaction.begin() would trigger this >>> IllegalStateException: >>> - >>> https://github.com/hibernate/hibernate-orm/blob/cf0fb8d262ec725a4d0692e13... >>> >>> I'll say that the new behaviour doesn't look unreasonable, but I'd >>> like to hear from you if this was intentional as you seemed to suggest >>> over chat that the accessTransaction() method was to be a drop-in >>> replacement for the previous semantics of getTransaction. >>> >>> Secondarily, when it comes to the "transaction.commit()" I'm having no >>> exception and it seems to work fine... should I need to check for the >>> state? >>> >>> Thanks, >>> Sanne >>> >>> >>> On 13 September 2016 at 15:54, Steve Ebersole <steve(a)hibernate.org&gt; >>> wrote: >>> > NIce! I never knew of this plugin, but there is a Gradle plugin for it >>> > as >>> > well. >>> > >>> > On Tue, Sep 13, 2016 at 9:33 AM Sanne Grinovero <sanne(a)hibernate.org > >>> > wrote: >>> >> >>> >> Since Hibernate ORM 5.2, the method getTransaction() on Session needs >>> >> to behave according to EntityManager spec, which implies that it has >>> >> to throw an exception in certain circumstances which depend on the >>> >> configuration. >>> >> >>> >> Hibernate Search used this method in various places, for example to >>> >> integrate with the current transaction's events, or even to control >>> >> the transaction explicitly in the case of the MassIndexer. >>> >> >>> >> Since we want Hibernate Search to work fine with Hibernate ORM no >>> >> matter what configuration is being used, we need to avoid invoking >>> >> this method. >>> >> The solution is extremely simple: use its SPI level replacement, which >>> >> is SessionImplementor#accessTransaction(). >>> >> >>> >> Unfortunately most of our Search/ORM tests happen to run without a >>> >> Transaction Manager so if you happen to use the old method, the tests >>> >> would pass and everything would seem fine - however your shiny new >>> >> feature would not work in certain configurations. >>> >> >>> >> One solution to verify we're not using it, is to ban this method using >>> >> the "forbiddenapi" plugin: >>> >> - >>> >> >>> >> https://github.com/Sanne/hibernate-search/commit/a980ee5dca0c7a58dd79ba98... >>> >> >>> >> A more comprehensive integration test would be to re-run all tests >>> >> from the Search/ORM using a proper JTA configuration; not rushing to >>> >> refactor our testsuite now since we have the forbidden-apis plugin but >>> >> opening a JIRA task for 5.7, as this version will support ORM 5.7: >>> >> - https://hibernate.atlassian.net/browse/HSEARCH-2344 >>> >> >>> >> Thanks, >>> >> Sanne >>> >> _______________________________________________ >>> >> hibernate-dev mailing list >>> >> hibernate-dev(a)lists.jboss.org >>> >> https://lists.jboss.org/mailman/listinfo/hibernate-dev