JBossDeveloper

Tom Jenkinson wrote:

 

Having thought some more about this, I think that calling the disassociated() method so late is a problem as it will never be called before afterCompletion - never giving JPA the chance to clean up the EM at the correct point. If we want to keep the facility within Narayana we need consider how to get the callback currently known as "disassociated()" called prior to the Sync::afterCompletion() calls.

Is this a concern about ordering of the afterCompletion calls?  Where the JPA afterCompletion callbacks would run later than the "registered to be called last sync"?

Or is this more about ensuring that the JPA callbacks are called before the thread is disassociated from the transaction (e.g. so JPA can access the TSR resource map and also  check tx status)?

Mark Little wrote:

 

Why were you thinking about AtomicAction.java?

The reason I mentioned AtomicAction and that line in particular is after the call to ThreadActionData.popAction() completes, clients will be prevented from accessing the data in the TSR. I shouldn't have said "immediately" but we may need to reconsider the naming of the callback if we execute the callback earlier than when the thread is internally disassociated (see comment 27 also).

Scott Marlow wrote:

 

Tom Jenkinson wrote:

 

Having thought some more about this, I think that calling the disassociated() method so late is a problem as it will never be called before afterCompletion - never giving JPA the chance to clean up the EM at the correct point. If we want to keep the facility within Narayana we need consider how to get the callback currently known as "disassociated()" called prior to the Sync::afterCompletion() calls.

 

Is this a concern about ordering of the afterCompletion calls?  Where the JPA afterCompletion callbacks would run later than the "registered to be called last sync"?

 

Or is this more about ensuring that the JPA callbacks are called before the thread is disassociated from the transaction (e.g. so JPA can access the TSR resource map and also  check tx status)?

Ideally I believe you want to be able to execute your afterCompletion code as normal. The issue you have today is concurrent access to the entitymanager close if the reaper is called. In the normal case I am expecting you would like the following:

That would be distinct from the timeout case:

I just want to point out that Synchronization::afterCompletion() operates in an undefined transaction context, as such I don't believe access to resources in the TSR are guaranteed to be available at that point.

OK, but not everything uses AtomicAction, e.g., JTS. Wouldn't it be better to have a solution that works across JTA implementations and is implemented once?

Personally I don't know if there is a TSR issue as whatever resources JPA needs might be able to be cached in the syncs beforeCompletion. What seemed strange was that the JPA afterCompletion callback wouldn't do anything and it would always be the disassociated() (or in ThreadAssociationControl terms one of "commit()||rollback()||suspend()") callbacks that did the work.

ThreadAssociationControl is JTS only at the moment so you would need quite a few changes to make that available to the local only implementation.

The only clear requirement I know is we are trying to support JPA Syncs avoiding concurrent calls to EntityManager.close() - I am not well versed on the TSR resource requirements of JPA and we need Scott to clarify those (including whether he expects to be able to access the resources in afterCompletion - and why he can't cache those resources in beforeCompletion).

Personally I don't know if there is a TSR issue as whatever resources JPA needs might be able to be cached in the syncs beforeCompletion. What seemed strange was that the JPA afterCompletion callback wouldn't do anything and it would always be the disassociated() (or in ThreadAssociationControl terms one of "commit()||rollback()||suspend()") callbacks that did the work.

For the rollback case, I don't expect beforeCompletion to be called. 

We do the clean up work at afterCompletion time, because the clean up does not involve writing to the database, instead, the clean up involves detaching entities from the persistence context and possibly closing the persistence context.  We cannot do either of these clean up actions in beforeCompletion, as that would be before the transaction has completed.

The only clear requirement I know is we are trying to support JPA Syncs avoiding concurrent calls to EntityManager.close() - I am not well versed on the TSR resource requirements of JPA and we need Scott to clarify those (including whether he expects to be able to access the resources in afterCompletion - and why he can't cache those resources in beforeCompletion).

There are no TSR resource requirements of JPA, as that is part of the JSR API.  When we move the JPA afterCompletion logic, to run in a different thread, we should try to ensure that the application thread environment is similar to the REAPER thread environment, with regard to the (rolled back) transaction associated with the application thread.

I think it was suggested before, that we wrap the TSR instance, which I think makes sense for this case (perhaps via a Jipijapa TSRWrapper).  I was thinking that the TSRWrapper would mostly delegate to the underlying (Narayana) TransactionSynchronizationRegistry but I suppose the TSRWrapper could maintain a separate resource map (for any resources that are added by the persistence provider or the EE-JPA container level).

Just to make sure everyone's on the same page: beforeCompletion is not called if the application or container calls rollback on the transaction. Of course it may be called if the transaction rolls back but commit was called initially and we eventually decide we have to roll back, but that's a different flow of control through the code.

What's the current thinking then?

I think that the IJ/JCA afterCompletion synchronization callback, also wants to be called in the foreground/application thread (so that it is the last sync called), instead of being called from the background (reaper) thread.  I'm not sure if in WildFly, when handling the timeout case, we should:

1. only run the JPA (WildFly EE/JPA container sync + Hibernate persistence provider) sync afterCompletion in the application thread.
2. run the JCA + JPA (WildFly EE/JPA container sync + Hibernate persistence provider) sync afterCompletion in the application thread.
3. Run all sync afterCompletion callbacks in the application thread.

Doing #1, introduces some risk, #2 introduces more and #3 even more.  The higher risk is from pushing more use cases through the WildFlyTransactionSynchronizationRegistryWrapper that maintains its own resource map, while delegating to the underlying TransactionSynchronizationRegistry.

Are there are any ordering cases that we need to guard against in the WildFlyTransactionSynchronizationRegistryWrapper, where the application thread does not run the callback syncs because we miss that the background thread is about to roll back the transaction?  Or the reaper thread misses that the application thread is in the middle of suspending the transaction during the reaper timeout handling?

Thanks. I agree that focussing on the agreed use case at this stage is the right thing to do.

As per c32, I think the TransactionEvent we add should be DISASSOCIATING. You can then call it before commit/rollback in BaseTransactionManagerDelegate so that Syncs that are watching for the event can (in the normal none-rollback case) execute their afterCompletion logic as normal.

It really depends on which WildFlyTransactionSynchronizationRegistryWrapper methods are delegated to the underlying TransactionSynchronizationRegistry and what internal TM resources still need to be associated with the application thread (implying the need for DISASSOCIATING, since more TM functionality is likely to work).  For example, I'd expect that the underlying TSR.getStatus would be used, rather than caching a potentially stale tx status in the WildFlyTransactionSynchronizationRegistryWrapper.