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

[Steve Ebersole]

That would be my personal opinion.


On Wed, Sep 14, 2016 at 11:24 AM Sanne Grinovero <sanne at hibernate.org>
wrote:

> On 14 September 2016 at 17:16, Steve Ebersole <steve at hibernate.org> 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 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
>