2017-06-12 10:29 GMT+02:00 Svetlin Zarev <[email protected]>:
> > why not relying on the > default? javax.transaction.Transaction.class.cast(txn). > registerSynchronization(listener > > Because it registers the transaction in the "regular" synchronizations > list. But > if the synchronization is registered via the > TransactionSynchronizationRegistry, > the synchronization will be registered in the "interposed" synchronization > list. > > The synchronizations from the interposed list are always executed after the > synchronizations from the regular list. > Makes sense. Note you can just get registerInterposedSynchronization from the txn directly, it would make less reflection and do exactly the same. > > OpenEJB's SessionSynchronizationCoordinator is registered into the > interposed list, so if we want for @BeforeCompletion to work > correctly with eclipselink , then either openejb's > SessionSynchronizationCoordinator > must be registered in the regular list, or eclipselink's synchronization > must be registered in the interposed list. I think that the second option > is less invasive. > Your patch (adapted with the previous reflection enhancement probably) looks good, just miss a test to ensure it works (surely in openejb-core with application composer) > > > 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <[email protected]>: > > > Hi Svetlin, > > > > why not relying on the > > default? javax.transaction.Transaction.class.cast(txn). > > registerSynchronization(listener)? > > This is what your code does except it goes through the registry to find > the > > current transaction instead of using the one already passed and bound to > > the entity manager - should lead to the same if the application doesnt > > abuse of transactions. > > > > The reflection is here cause openejb-core depends on > > openejb-jpa-integration so if you add openejb-core as a dependency it > > wouldn't build. This is done cause jpa-integration is added to > applications > > for the ones providing the jpa provider instead of using the container > one. > > > > Side note: would be good to ensure any PR has some test(s) when possible > > otherwise it is easy to break it before next release without even > noticing > > it. > > > > > > > > Romain Manni-Bucau > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > <https://blog-rmannibucau.rhcloud.com> | Old Blog > > <http://rmannibucau.wordpress.com> | Github <https://github.com/ > > rmannibucau> | > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory > > <https://javaeefactory-rmannibucau.rhcloud.com> > > > > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail. > com > > >: > > > > > Hi Everyone, Romain, > > > > > > As JIRA is down, I'm writing to the mailing list. > > > > > > Recently I reported TOMEE-2057 -> modification to > > > entities done in the @BeforeCompletion callback were > > > not getting persisted. The root cause was that > > > eclipselink's synchronization was executed before > > > openejb's one. The same case works as expected > > > with OpenJPA. > > > > > > Hence I want to propose the following fix for the > > > eclipselink's integration with tomee: [1] OpenJPA > > > does the same, but it's implemented inside OpenJPA. > > > > > > [1] https://github.com/apache/tomee/pull/70 > > > > > > PS: Is there really need for reflection ? Why don't we add > > > dependency to OpenEJB-Core and remove the reflection ? > > > > > > Best regards, > > > Svetlin > > > > > >
