JBossDeveloper

Michael Musgrove wrote:

 

Mark Little wrote:

 

A "reason" field? Seems like overkill. Why not just make the event type more explicit then? SUSPENDING, COMMITTING, ROLLINGBACK, ..?

Also, the reason I suggested a reason field was to avoid having multiple types for the same underlying event. For example the notifications DISASSOCIATING and SUSPENDING are produced when suspend() is called so semantically it is one event and there are different reasons for that event.

Its why I suggested ORing the values as they are all states. We don't really want onEvent() xN calls

I am not sure why we need aborting/committing as a synchronization can provide a record of that?

UserTransactionListener can tell you when a UT is created - I guess you would need to check if that is only called back to listeners registered with the current thread. IIRC the intention was the user set the listener on a transaction, so calling back on begin does not work as that has already happened.

You need feedback from the EE component leads on this - wrapping a BMT/EJB in its own "clean" push/pop context would be a valid scenario if there is 0 or 1 UserTransaction lifecycle associated with it, so no callbacks would be needed.

IronJacamar doesn't know anything about component models above it, and how they relate to transaction boundaries. Hence, it is the component containers above that needs to define when push/pop are called and thereby the requirements for TransactionLifecycleEventType - not me.

The standard lazy enlistment scenarios are working - otherwise we wouldn't be certified - IronJacamar uses UserTransactionListener for that - which is enough.

Actually, the BMT/EJB case is a bad example, as it always needs to have a clean CCM context to operate in. Maybe after suspending, and before resuming hooks are enough for all EE containers...

Maybe Stephen can share some light on the cases he has been investigating.

After suspending and before resuming, is the opposite of what we need for JPA (persistence providers depend on the transaction still being associated with the thread).

Sounds like we might need all four: ASSOCIATING, ASSOCIATED, DISASSOCIATING, DISASSOCIATED

In the absence of further input - the only concrete requirement I can see being consumed at this time is ASSOCIATED/DISASSOCIATING. I think we should proceed and when any other consumers of the API are identified we will need to tackle that. I am somewhat concerned though that this API is thread association related and that other use cases may be transaction lifecycle related, Mikes solution would address that or alternatively we could rename:

ASSOCIATED -> RESUMED

DISASSOCIATING -> SUSPENDING, COMMITTING and ROLLINGBACK

and rather focus this at a transactions status now.

Why not use bit masks? For example, take a 64 bit value and have the higher order 32 bits represent the event and the lower order 32 bits the "reason"? It's pretty extensible without changing the interfaces later.

tomjenkinson wrote:

 

In the absence of further input - the only concrete requirement I can see being consumed at this time is ASSOCIATED/DISASSOCIATING. I think we should proceed and when any other consumers of the API are identified we will need to tackle that. I am somewhat concerned though that this API is thread association related and that other use cases may be transaction lifecycle related, Mikes solution would address that or alternatively we could rename:

ASSOCIATED -> RESUMED

DISASSOCIATING -> SUSPENDING, COMMITTING and ROLLINGBACK

and rather focus this at a transactions status now.

Reconsidering this, those status wouldn't be appropriate actually. Scott needs the callbacks when the thread is associated/disassociated not committing/rolling back otherwise (as Mike points out) he could use a synchronization. The reason he can't use a synchronization is because those are ran relative to the AtomicAction and not the app servers representation (TransactionImple). Changing the callback to COMMITTING/ROLLINGBACK would be misleading as the internal AtomicAction may have been timed out too long ago and to see this status flag up after seems strange and counterintuitive.

I think we are in danger of losing a solution if we try to make this too general right now. Personally so long as we have a mechanism to flag multiple states at the same time then in the future should it be OK to add the states for the use case we know (ASSOCIATED/DISASSCOIATING) and add others as users come to us with specific requests. Categorising them as subclasses of EventType is possible but I am not sure whether the hierarchy adds too much value over adding other entries directly to EventType.

The API probably doesn't need the TransactionListenerRegistry.addListener(TransactionListener) method as that could include more events (in the future) than someone codes for.  IMO, safer to just include the addLIstener(TransactionListener, EnumSet<EventType>). 

For TransactionEvent.getType(), isn't that expected to return the current even state?  So the TransactionListener can code something like:

@Override
public synchronized void onEvent(TransactionEvent transactionEvent) {
    if (transactionEvent.getType().equals(EventType.ASSOCIATED)) {
        disassocCalled = false;
    }
    else if (transactionEvent.getType().equals(EventType.DISASSOCIATING) {
        disassocCalled = true;
        if (safeToClose()) {
            entityManager.clear();
        }
    }
}

private boolean safeToClose() {
  return afterCompletionCalled == true && disassocCalled == true;
}