[mmusgrov]

This sounds like feature creep. We agreed that the ASSOCIATED and DISASSOCIATED event callbacks were sufficient for your use case. Of course it is straightforward to add a third one for DISASSOCIATING so if you are saying that your implementation needs that extra event then we can certainly include it but it is adding more overhead to each transaction. If you do need it maybe we can look at make the registration process more fine grained so that listeners may listen for a subset of the available events to avoid superfluous callback invocations.

[mmusgrov]

Here's a suggestion. I will commit a (SNAPSHOT) version that includes all 3 events and then you can prototype your solution. If after that exercise we discover that we only require a subset of the events then we can prune accordingly.

[smarlow]

Sure, that makes sense.  Thanks Mike.

[tomjenkinson]

I honestly can't see a use case for DISASSOCIATE, I think the API should be ASSOCIATED and DISASSOCIATING for the reasons I have given earlier. I think we should proceed with those two call backs and await feedback if something does prove missing.

The status of the transaction is provided in the afterCompletion callback.

[tomjenkinson]

The only use case I know is to guard the manager.close in here:

https://github.com/wildfly/wildfly/blob/master/jpa/src/main/java/org/jboss/as/jpa/transaction/TransactionUtil.java#L150

If we use DISASSOCIATING, then all that needs to be changed is safeToClose() can use a check to see if DISASSOCIATING was called already instead of the outcome.

The implementation in JPA should be something like:

synchronized void stateChange(javax.transaction.Transaction transaction, TransactionEvent transactionEvent) {

  if (transactionEvent == TransactionEvent.ASSOCIATED) {

     disassocCalled  = false;

  } else {

    diassocCalled = true;

    if (safeToClose()) {

      manager.close();

    }
  }

}

synchronized void afterCompletion(int status) {

  acCalled = true;

  if (safeToClose()) {

    manager.close();

  }

}

void safeToClose() {

  return acCalled == true && disassocCalled = true;

}

If we use DISASSOCIATED instead the implementation would be similar but the AC would never do the manager.close() which doesn't seem ideal to me.

We are really talking about which side of narayana/BaseTransactionManagerDelegate.java at master · jbosstm/narayana · GitHub (and suspend/rollback) to invoke this callback. What I am saying is that calling it before AC calls is best as less needs to change.

[marklittle]

I agree with Tom. Let's keep this focused on what we know we need today.

[mmusgrov]

So the plan is to satisfy Scotts requirements by supporting notifications for ASSOCIATED which is the resume (and the initial register of the callback) and DISASSOCIATING which includes commit, rollback and suspend. The notification invokes the listener with the transaction and the EventType (ASSOCIATED or DISASSOCIATING).

But Jesper has a use case where we need these cases separated - e.g. begin() != resume(), and commit() != rollback() != suspend().

I propose that we support this use case by adding a "reason" field to the notification payload where reason is one of begin/suspend/resume/commit/rollback. Is this good enough?

[tomjenkinson]

Can we OR the status types? SUSPENDING|DISASSOCIATING.

I would rather not add the reason type as that would suggest that the API is not able to be extended easily in the future in case we come up with something else.

[marklittle]

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

[mmusgrov]

Earlier posts here have expressed a requirement for extensibility but Java enums are final classes so I have opted for the following proposal instead:

// base interface for event types

  public interface EventType { String getName(); }

// register interest in one or more event types

  public void addListener(TransactionListener listener, EventType ... types);

// registration process

  public interface TransactionListenerRegistry {

     void addListener (TransactionListener listener, EventType ... types);

     boolean isSupported(EventType type);

  }

// event types for thread association

  public enum ThreadAssociationEventType implements EventType {

    ASSOCIATED, DISASSOCIATING;

  }

// event types for the transaction lifecycle

  public enum TransactionLifecycleEventType implements EventType {

    BEGIN, ABORTING, COMMITTING ;

  }

Sott needs ThreadAssociationEventType and Jesper wants TransactionLifecycleEventType

If we need more events then we will implement more EventTypes for them.

If we can agreement soon we can hit the release that Tom is about to do.

[mmusgrov]

mmusgrov wrote:

// event types for thread association

  public enum ThreadAssociationEventType implements EventType {

    ASSOCIATED, DISASSOCIATING;

  }

// event types for the transaction lifecycle

  public enum TransactionLifecycleEventType implements EventType {

    BEGIN, ABORTING, COMMITTING ;

  }

TransactionLifecycleEventType is probably not enough for Jespers use case. Jesper, which subset of {SUSPENDING, SUSPEND, RESUMING, RESUME, COMMITTING, ABORTING} do you need ( ABORTED and COMMITTED) are available via synchronizations.

[mmusgrov]

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.

[mmusgrov]

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

As a side note, earlier posts on this thread did propose a more complete set of states but that approach was abandoned using the argument that we should:

• only provide enough states for the known use cases;
• make sure the event type state space and implementation is extensible so that we can support future use cases;

[jesper.pedersen]

You need to account for the states that are exposed to the users. If a component inside WildFly exposes UserTransaction or even TransactionManager then it means that you need to account for those states including the suspending + resuming an existing transaction. Think a bean managed transaction EJB, if the user does a lazy enlist of a connection inside the wrong cached connection manager context will be used.

BMT/EJB               CONTAINER
                      suspend existing tx
Connection c = ds..
                      push new CCM context
ut.begin();
// do work
ut.commit();
                      pop CCM context
c.close()
                      resume old tx

So, here you will need to provide the EJB container with callbacks to allow it to implement them - note, that there are both pre- and post- actions. Stuart/David can likely provide an overview of what is legal in an EJB / WEB context...

[tomjenkinson]

Jesper Pedersen wrote:

You need to account for the states that are exposed to the users. If a component inside WildFly exposes UserTransaction or even TransactionManager then it means that you need to account for those states including the suspending + resuming an existing transaction. Think a bean managed transaction EJB, if the user does a lazy enlist of a connection inside the wrong cached connection manager context will be used.

1. BMT/EJB               CONTAINER 
2.                       suspend existing tx 
3. Connection c = ds.. 
4.                       push new CCM context 
5. ut.begin(); 
6. // do work 
7. ut.commit(); 
8.                       pop CCM context 
9. c.close() 
10.                       resume old tx 

So, here you will need to provide the EJB container with callbacks to allow it to implement them - note, that there are both pre- and post- actions. Stuart/David can likely provide an overview of what is legal in an EJB / WEB context...

I am not sure if Mike followed the above but can you tell us which callback you would need on which line - its not clear to me sorry.