Requirement for an observer SPI for tracking thread-to-transaction association changes

[mmusgrov]

This is the follow up to the thread Re: Design Discussion: Changing the reaper to use setRollbackOnly() instead of rollback() where the requirement for a new SPI for clients such as JPA and IJ to listen for thread-to-transaction association changes arose. Comment number 72 summarises the current proposals for the new SPI which I will repeat here:

Option 1)

  public interface ThreadAssociationListener extends Synchronization {

    public void threadAssociated();

    public void threadDisassociated();

  }

A somewhat improved interface exposes a simple extensible state model:

Option 2)

  interface ExtendedTransactionListener {

    void stateChange(TransactionEvent e);

  }

  enum TransactionEvent {

    ASSOCIATED,

    DISASSOCIATED

  }

Here we shy away from adding any state changes that are already provided by javax.transaction.Synchronization.

This option is nice because:

- it is sufficient to addresses the problem in hand, and

- it is extensible in the event of future requirements

Jesper argues for a more generic approach (to avoid having to revisit it if future requirements crop up in this area):

Option 3)

Something along the lines of:

interface TransactionLifecycleListener {

    void before(TransactionLifecycleEvent e);

    void after(TransactionLifecycleEvent e);

}

enum TransactionLifecycleEvent {

  ASSOCIATED,

  START,

  PREPARE, NO_PREPARE /* in case there is no prepare() call */

  ...,

  BEFORE_COMPLETION,

  AFTER_COMPLETION,

  DISASSOCIATED

}

The TransactionLifecycleListener methods could even take a Object... payload parameter such that the implementation could do something like:

after:

  if (ENLIST.equals(e)) {

    XAResource xares = payload[0];

    // do stuff

  }

This option needs further discussion:

- why do we need before and after

- why do we need the extra lifecycle callbacks (such as the implied ENLIST event, what is START), some use cases would be desirable;

- should we be duplicating lifecycle callbacks that are already available via javax.transaction.synchronization.

[marklittle]

Is there a clear need for any extensibility at this stage?

[tomjenkinson]

The Jira for the use case is summarized here based on community feedback from the other thread: [JBTM-2342] Add callbacks for thread-to-transaction association changes - JBoss Issue Tracker

The following diagram illustrates the call backs you could expect when executing in a 2 JVM scenario where JVM1 calls JVM2 twice within the scope of the same transaction:

The following diagram explains the proposed call backs if say a reaper in JVM1 times out the TX:

As you can see, it should allow a Synchronization/TXAssociationListener combo to synchronize themselves and ensure they do not tamper with a none-thread safe transactional resource if the only threads are the app thread and the transaction reaper.

[tomjenkinson]

Mark Little wrote:

Is there a clear need for any extensibility at this stage?

+1 - I am not aware of any need for any further extensibility at this stage. The safest design would be something that tackles the issue we are working to resolve for the user today but does not preclude additions in the future. As such I would propose an interface name that captures the fact that this is an extension to Synchronization with additional callbacks.

How about:

interface ExtendedSynchronization {

public void stateChange(TransactionEvent e);

}

enum TransactionEvent {

  ASSOCIATED,

  DISASSOCIATED

}

That should be enough to capture the events we know we need. It also means in the future that if we discovered a need to flag to users an event was about to happen you could add beforeStateChange(TransactionEvent) to the interface. And if we found a need for future events we can add them to the enum.

[tomjenkinson]

In terms of naming, we could even go with:

interface ExtendedSynchronization extends javax.transaction.Synchronization

[tomjenkinson]

Also, as a note to how to register one of these things. We have two immediate options without changing the JTA API:

1. Use putResource in the TSR

2. Do an "instanceof ExtendedSynchronization" during register*Synchronization methods

[tomjenkinson]

Of course, my answers 4 and 5 assume a certain level of coupling to JTA, if we wanted to remain neutral to that we don't need the "extends javax.transaction.Synchronization" but the rest can still hold. We could also provide something on BasicAction to pass one of these in if the user is coding directly against that.

[marklittle]

I'm no longer sure if it makes sense to use ThreadActionData, which is what I was thinking earlier today. However, I do wonder what the overhead implications are on doing these additional calls, especially on transactions that don't have a need. One obvious optimisation is not to do this unless there are Synchronisations registered.

[mmusgrov]

You could keep the idea of using ThreadActionData if ExtendedSynchronization registrations are stored with the BasicAction. Then when BasicAction#removeChildThread() and BasicAction#addChildThread() are called in response to suspend and resume we would notify the listeners in this list of ExtendedSynchronizations. That sounds like a rather simple change to make to support the new feature.

[marklittle]

Yes, but I'm worried about overhead on additional calls. Plus, BasicAction doesn't know about Synchronizations at all - that concept doesn't come in until TwoPhaseCoordinator. It would break the model to add them to BasicAction.

[mmusgrov]

But we are talking about thread association listeners (not synchronisations - we don't have to call them ExtendedSynchronizations) and these rightly belong to BasicAction.

The additional calls need to  be made somewhere if we want to support the feature

[marklittle]

Wouldn't make sense to add this to BA if it looks and feels like a synchronisation specific thing but is called Fred, for instance.

And yes of course additional calls need to be made somewhere, but no one has yet responded to my concern over any potential overhead. Optimisations and location of these calls play into that discussion.

[mmusgrov]

Mark Little wrote:

Wouldn't make sense to add this to BA if it looks and feels like a synchronisation specific thing but is called Fred, for instance.

Why are thread association changes synchronisation specific. If you argue that they are then the logical consequence would be that ThreadActionData calls into BasicAction are misplaced.

Mark Little wrote:

And yes of course additional calls need to be made somewhere, but no one has yet responded to my concern over any potential overhead. Optimisations and location of these calls play into that discussion.

Since suspend/resume handling happens in BasicAction and we want to add notifications for when that happens what alternative is there?

[mmusgrov]

Mark Little wrote:

And yes of course additional calls need to be made somewhere, but no one has yet responded to my concern over any potential overhead. Optimisations and location of these calls play into that discussion.

Lcating the calls in AtomicAction could be made to work too if BA really is too high up in the hierarchy.

[marklittle]

Michael Musgrove wrote:

Mark Little wrote:

Wouldn't make sense to add this to BA if it looks and feels like a synchronisation specific thing but is called Fred, for instance.

Why are thread association changes synchronisation specific. If you argue that they are then the logical consequence would be that ThreadActionData calls into BasicAction are misplaced.

I didn't say thread association was synchronisation specific. What I meant was really two things: first if the class/interface has a name which implies synchronisations then it shouldn't go into BA; second if the class/interface has extensions that only make sense for, or imply, synchronisations then it shouldn't into BA.

Mark Little wrote:

And yes of course additional calls need to be made somewhere, but no one has yet responded to my concern over any potential overhead. Optimisations and location of these calls play into that discussion.

Since suspend/resume handling happens in BasicAction and we want to add notifications for when that happens what alternative is there?

Well this is part of the problem. If you want to do this generically then ever time a transaction is associated or disassociated there'd be a call on this class to go and do work, just as there is with ThreadActionData. That's potentially an unnecessary overhead that we need to examine first. There would be little scope for optimisations here other than assume most of the time it's a null list of participants that will be trawled (or not). If we make this specific to synchronisations then there are optimisations that may be made, such as only calling if there is at least one synchronisation registered and perhaps only calling if disassociation/association occurs at specific points in a flow.