[tomjenkinson]

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.

[jesper.pedersen]

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.

[jesper.pedersen]

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.

[smarlow]

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

[tomjenkinson]

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.

[marklittle]

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.

[mmusgrov]

Tom Jenkinson 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.

So for now I will provide

  // event types for a subset of the transaction lifecycle

  // does anyone have a good name for this class

  public enum ASubsetOfTransactionLifecycleEventTypes implements EventType {

   SUSPENDING, RESUMED, COMMITTING, ABORTING

  }

and drop the event types for thread association. The interface I proposed in comment #54 is extensible so we can still address other states as the need arises.

[tomjenkinson]

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.

[mmusgrov]

Good observations Tom. So we will stick with the original solution (the one that Scott has already been testing against) and add extra enum constants to EventType as and when new use cases arise.

I will add an extra operation (or perhaps change the signature of the current one) to TransactionListenerRegistry to allow registration for a subset of event types (using an EnumSet):

public interface TransactionListenerRegistry {

   void addListener (TransactionListener listener, EnumSet<EventType> types); // listen for a selection of events

   void addListener (TransactionListener listener); // listen for all events - Scott is this method still useful since it is available via the new addListener method

}

If we need to flag multiple states at the same time then I can an extra getter (or change the signature of the current one) to TransactionEvent:

public class TransactionEvent {

    public EnumSet<EventType> getTypes() ... // return all states for this event

    public EventType getType() ... // return the first state - Scott is this method still necessary since it is available via this new getTypes method

}

[smarlow]

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;
}

[mmusgrov]

Scott Marlow wrote:

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

I included TransactionEvent.getTypes() because there may be more than one state per event. For example if, in the future, we have a requirement for the state ABORTING then the notification would include both EventType.DISASSOCIATING and EventType.ABORTING so your example would look like:

@Override
public synchronized void onEvent(TransactionEvent transactionEvent) {

  if (transactionEvent.getTypes().contains(EventType.ASSOCIATED))

  ...

  else if (transactionEvent.getTypes().contains(EventType.DISASSOCIATING)

  ...