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

[Steve Ebersole]

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.


On Wed, Sep 14, 2016 at 11:15 AM Steve Ebersole <steve at hibernate.org> 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 at hibernate.org>
> 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/cf0fb8d262ec725a4d0692e13d0a56d149d84584/hibernate-core/src/main/java/org/hibernate/engine/transaction/internal/TransactionImpl.java#L50-L53
>>
>> 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 at hibernate.org>
>> 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 at 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/a980ee5dca0c7a58dd79ba98acd8a354bc5601e6#diff-600376dffeb79835ede4a0b285078036R1036
>> >>
>> >> 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 at lists.jboss.org
>> >> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>
>