Hi! Any comment on this? After reading the javadoc of SharedSessionContractImplementor, I think we should probably just get rid of the `checkTransactionSynchStatus();` call in getInterceptor(). I don't think getInterceptor() should be responsible for joining the transaction. What do you all think?

-- Guillaume On Fri, Sep 8, 2017 at 6:26 PM, Guillaume Smet <guillaume.smet(a)gmail.com&gt; wrote: > Hi, > > Note to Gail: this is potentially a blocking issue for 5.2.11 for the OGM > upgrade (I thought it was a bug in OGM but apparently, it's an issue with > ORM). > > In 5.2 SessionImpl, we now use getInterceptor() instead of accessing the > interceptor field directly because the field has been moved to > AbstractSharedSessionContract. > > See for instance the change made here in afterTransactionCompletion(): > https://github.com/hibernate/hibernate-orm/blame/master/ > hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java#L2443 > > I think this is an issue as getInterceptor() is not a simple getter but is: > @Override > public Interceptor getInterceptor() { > checkTransactionSynchStatus(); > return interceptor; > } > > protected void checkTransactionSynchStatus() { > pulseTransactionCoordinator(); > delayedAfterCompletion(); > } > > Thus calling the pulse() method of the TransactionCoordinator, triggering > an implicit join whereas we're in the afterTransactionCompletion() phase. > > This is an issue for us as the pulse() method of our > TransactionCoordinator creates Neo4j transactions so when the > getInterceptor() method is called in afterTransactionCompletion(), we > create a new Neo4j transaction. > > So 2 questions: > - should we really call checkTransactionSynchStatus(); in > getInterceptor()? If feels a bit weird. > - if so, I think we should have a true protected getter (interceptor() > following Steve's convention?) to avoid SessionImpl "pulsing" the > transaction coordinator when accessing the interceptor. > > Thanks for your feedback! > > -- > Guillaume > _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

I'm fine with that. That method really does not fit with the paradigm which needs that pulse. On Mon, Sep 11, 2017 at 1:36 PM Sanne Grinovero <sanne(a)hibernate.org&gt; wrote: > On 11 September 2017 at 16:14, Guillaume Smet <guillaume.smet(a)gmail.com&gt; > wrote: > > Hi! > > > > Any comment on this? > > > > After reading the javadoc of SharedSessionContractImplementor, I think we > > should probably just get rid of the `checkTransactionSynchStatus();` call > > in getInterceptor(). > > > > I don't think getInterceptor() should be responsible for joining the > > transaction. > > > > What do you all think? > > +1 > > > > > -- > > Guillaume > > > > On Fri, Sep 8, 2017 at 6:26 PM, Guillaume Smet < guillaume.smet(a)gmail.com > > > > wrote: > > > >> Hi, > >> > >> Note to Gail: this is potentially a blocking issue for 5.2.11 for the > OGM > >> upgrade (I thought it was a bug in OGM but apparently, it's an issue > with > >> ORM). > >> > >> In 5.2 SessionImpl, we now use getInterceptor() instead of accessing the > >> interceptor field directly because the field has been moved to > >> AbstractSharedSessionContract. > >> > >> See for instance the change made here in afterTransactionCompletion(): > >> https://github.com/hibernate/hibernate-orm/blame/master/ > >> > hibernate-core/src/main/java/org/hibernate/internal/ SessionImpl.java#L2443 > >> > >> I think this is an issue as getInterceptor() is not a simple getter but > is: > >> @Override > >> public Interceptor getInterceptor() { > >> checkTransactionSynchStatus(); > >> return interceptor; > >> } > >> > >> protected void checkTransactionSynchStatus() { > >> pulseTransactionCoordinator(); > >> delayedAfterCompletion(); > >> } > >> > >> Thus calling the pulse() method of the TransactionCoordinator, > triggering > >> an implicit join whereas we're in the afterTransactionCompletion() > phase. > >> > >> This is an issue for us as the pulse() method of our > >> TransactionCoordinator creates Neo4j transactions so when the > >> getInterceptor() method is called in afterTransactionCompletion(), we > >> create a new Neo4j transaction. > >> > >> So 2 questions: > >> - should we really call checkTransactionSynchStatus(); in > >> getInterceptor()? If feels a bit weird. > >> - if so, I think we should have a true protected getter (interceptor() > >> following Steve's convention?) to avoid SessionImpl "pulsing" the > >> transaction coordinator when accessing the interceptor. > >> > >> Thanks for your feedback! > >> > >> -- > >> Guillaume > >> > > _______________________________________________ > > hibernate-dev mailing list > > hibernate-dev(a)lists.jboss.org > > https://lists.jboss.org/mailman/listinfo/hibernate-dev > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev > _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

I created https://hibernate.atlassian.net/browse/HHH-11982. I just committed the one-line change without a unit test. Unless I hear otherwise, I'll assume that's OK and I'll plan to release on Wednesday, 9/13. Regards, Gail On Mon, Sep 11, 2017 at 10:52 PM, Gail Badner <gbadner(a)redhat.com&gt; wrote: > OK, I'll fix it for 5.2.11. > > Thanks, > Gail > > On Mon, Sep 11, 2017 at 12:20 PM, Steve Ebersole <steve(a)hibernate.org&gt; > wrote: > >> I'm fine with that. That method really does not fit with the paradigm >> which needs that pulse. >> >> >> On Mon, Sep 11, 2017 at 1:36 PM Sanne Grinovero <sanne(a)hibernate.org&gt; >> wrote: >> >> > On 11 September 2017 at 16:14, Guillaume Smet < >> guillaume.smet(a)gmail.com&gt; >> > wrote: >> > > Hi! >> > > >> > > Any comment on this? >> > > >> > > After reading the javadoc of SharedSessionContractImplementor, I >> think we >> > > should probably just get rid of the `checkTransactionSynchStatus();` >> call >> > > in getInterceptor(). >> > > >> > > I don't think getInterceptor() should be responsible for joining the >> > > transaction. >> > > >> > > What do you all think? >> > >> > +1 >> > >> > > >> > > -- >> > > Guillaume >> > > >> > > On Fri, Sep 8, 2017 at 6:26 PM, Guillaume Smet < >> guillaume.smet(a)gmail.com >> > > >> > > wrote: >> > > >> > >> Hi, >> > >> >> > >> Note to Gail: this is potentially a blocking issue for 5.2.11 for >> the >> > OGM >> > >> upgrade (I thought it was a bug in OGM but apparently, it's an issue >> > with >> > >> ORM). >> > >> >> > >> In 5.2 SessionImpl, we now use getInterceptor() instead of >> accessing the >> > >> interceptor field directly because the field has been moved to >> > >> AbstractSharedSessionContract. >> > >> >> > >> See for instance the change made here in >> afterTransactionCompletion(): >> > >> https://github.com/hibernate/hibernate-orm/blame/master/ >> > >> >> > hibernate-core/src/main/java/org/hibernate/internal/SessionI >> mpl.java#L2443 >> > >> >> > >> I think this is an issue as getInterceptor() is not a simple getter >> but >> > is: >> > >> @Override >> > >> public Interceptor getInterceptor() { >> > >> checkTransactionSynchStatus(); >> > >> return interceptor; >> > >> } >> > >> >> > >> protected void checkTransactionSynchStatus() { >> > >> pulseTransactionCoordinator(); >> > >> delayedAfterCompletion(); >> > >> } >> > >> >> > >> Thus calling the pulse() method of the TransactionCoordinator, >> > triggering >> > >> an implicit join whereas we're in the afterTransactionCompletion() >> > phase. >> > >> >> > >> This is an issue for us as the pulse() method of our >> > >> TransactionCoordinator creates Neo4j transactions so when the >> > >> getInterceptor() method is called in afterTransactionCompletion(), >> we >> > >> create a new Neo4j transaction. >> > >> >> > >> So 2 questions: >> > >> - should we really call checkTransactionSynchStatus(); in >> > >> getInterceptor()? If feels a bit weird. >> > >> - if so, I think we should have a true protected getter >> (interceptor() >> > >> following Steve's convention?) to avoid SessionImpl "pulsing" the >> > >> transaction coordinator when accessing the interceptor. >> > >> >> > >> Thanks for your feedback! >> > >> >> > >> -- >> > >> Guillaume >> > >> >> > > _______________________________________________ >> > > hibernate-dev mailing list >> > > hibernate-dev(a)lists.jboss.org >> > > https://lists.jboss.org/mailman/listinfo/hibernate-dev >> > _______________________________________________ >> > hibernate-dev mailing list >> > hibernate-dev(a)lists.jboss.org >> > https://lists.jboss.org/mailman/listinfo/hibernate-dev >> > >> _______________________________________________ >> hibernate-dev mailing list >> hibernate-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/hibernate-dev >> > >