[smarlow]

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)?

[tomjenkinson]

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).

[tomjenkinson]

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.

[marklittle]

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?

[mmusgrov]

The solution you gave me in comment #16 ThreadAssociationControl.java looks like a good way to go. I am just waiting for Tom and Scott to solve the TSR resource issue.

[tomjenkinson]

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).

[smarlow]

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).

[marklittle]

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.

[marklittle]

What's the current thinking then?

[smarlow]

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?

[mmusgrov]

The only use case we have for the feature is coming from wildfly so I propose we put the interfaces in the jboss-transaction-spi. The generic approach mentioned in comment #16 doesn't quite work since we would need to key the listener map by transaction instead of by thread but JTA and JTS transactions do not inherit from a common base class.

The proposal is:

public enum TransactionEvent {

    ASSOCIATED,

    DISASSOCIATED

}

public interface TransactionListener {

    /**

     * The callback for notifying registered listeners of a transaction related event

     * @param transaction the transaction that the event relates to

     * @param transactionEvent indication of what kind of change has occurred

     */

    void stateChange(javax.transaction.Transaction transaction, TransactionEvent transactionEvent);

}

and the interface for adding listeners is:

public interface TransactionListenerRegistry {

    void addListener (Transaction transaction, TransactionListener listener) throws SystemException;

}

BaseTransactionManagerDelegate will implement the interface and we will expose it via a wildfly service which can be injected into the JPA subsystem.

[marklittle]

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

[tomjenkinson]

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.

[mmusgrov]

Is this so that the TSR is always available in afterCompletion? Scott says he doesn't need that so what is wrong with the design we have already agreed on this thread where the call stateChange(TX,DISASSOCIATED) is made after application thread has committed (or rolled back) which is similar to the timeout case in comment 32. So, instead of your sequence diagram, we have the following:

[smarlow]

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.