Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduces LocalXAResource and a few support classes in jta/jdbc #5733

Merged
merged 1 commit into from
Dec 23, 2022

Conversation

ljnelson
Copy link
Member

Signed-off-by: Laird Nelson [email protected]

@ljnelson ljnelson added jpa/jta 3.x Issues for 3.x version branch labels Dec 20, 2022
@ljnelson ljnelson self-assigned this Dec 20, 2022
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Dec 20, 2022
@ljnelson
Copy link
Member Author

Another little grab bag pulled from my private branch work.

This introduces the LocalXAResource class, which is an XAResource implementation that adapts a non-JTA-aware Connection to the XA protocol.

The other classes introduced by this PR are supporting in nature and fairly simple.

More PRs in this area to follow.

*
* <p>Instances of this class are safe for concurrent use by multiple threads.</p>
*/
final class LocalXAResource implements XAResource {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, reviewers. I'll annotate this PR myself with comments like this to help you navigate the XA world.



@Override // XAResource
public void start(Xid xid, int flags) throws XAException {
Copy link
Member Author

@ljnelson ljnelson Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The XA protocol—the permitted sequence of invocations on an XAResource—begins with start. start's job is to mark the beginning of the association of operations on the current thread against the "resource manager" (the database connection in this case) with the global transaction identified by the supplied Xid. (The ending of this thread association is accomplished by the end method.)

The transaction manager can call start with a variety of flags, indicating whether a new transaction is starting or a previously suspended transaction is resuming, or, somewhat oddly, whether the transaction manager has decided that this XAResource represents the same resource manager as another XAResource, in which case one of them is said to "join". Why the XAResource interface didn't have a join or a resume method instead is beyond me, but here we are.

There are extremely specific rules about how to deal with violations of the XA protocol, all of which involve throwing an XAException with a specific error code.

.initCause(new IllegalArgumentException("xid: " + xid + "; flags: " + flagsToString(flags)));
}

this.computeAssociation(XARoutine.START, xid, remappingFunction);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We select the appropriate BiFunction to pass in to the computeAssociation method. As hopefully you can see above, it's determined by what switches the transaction manager has passed when it called this start method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand the role of remappingFunction. What has been "mapped" that will be in need of "remapping"? Is it the Association that is being remapped as a result of the about-to-happen state transition?

If so I'm not sure I have a good alternative for the name. It just seemed like a mild mismatch between the variable name and the function names being assigned to it but not a big deal.

Copy link
Member Author

@ljnelson ljnelson Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Remapping" in the sense documented here: https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/util/concurrent/ConcurrentMap.html#compute(K,java.util.function.BiFunction)

I use ConcurrentMap#compute(Object, BiFunction) because for some invocations of, say, start the Association (the value) will not exist, and will need to be created, and in other invocations the Association will exist (i.e. in the case of joining) and will need to be replaced with a new one in an appropriate new state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK. I was trying to make "remapping" fit the vocabulary of XA rather than of the data structure API you're using.

}

// (Remapping BiFunction, used in start() above and supplied to computeAssociation() below.)
private Association start(Xid x, Association a) {
Copy link
Member Author

@ljnelson ljnelson Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the start method above, if we were called with the right set of flags, then we are truly beginning a new transaction. The Association object is an inner class (see down at the end of this file) representing the association of a given Connection (in this XAResource implementation's case) with the current thread. So this method is a BiFunction that we selected in the start method above and is called by the computeAssociation method below.

Connection c;
try {
c = this.connectionFunction.apply(x);
} catch (RuntimeException e) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figuring out whether to throw an XAException with XAER_RMERR or XAER_RMFAIL is a problem that has resulted in dozens of bugs by all major XAResource implementors over a couple of decades. It is far from straightforward and ends up being a de facto decision: you have to do what Narayana expects, regardless of whether it implements the specification properly or not, because it is the only real robust JTA implementation out there and has encountered just about everything in its 30+ years of existence.

}

@Override // XAResource
public void end(Xid xid, int flags) throws XAException {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the transaction manager decides that whatever has gone on on the current thread should stop being associated with the global JTA transaction, it will call this method. Once again the flags tell you what sort of operation is really being invoked: a disassociation-from-the-current-thread due to an ordinary transaction boundary, a problem or a suspension.

}

// Any XAException thrown can have any error code. The transaction will be marked as rollback only. See
// https://github.com/jbosstm/narayana/blob/8ccaf0f85c7a76c227941d26cc3aa3fa9f05b160/ArjunaJTA/jta/classes/com/arjuna/ats/internal/jta/transaction/arjunacore/TransactionImple.java#L978-L992.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again, figuring out what kind of error code to throw when is trial and error in the real world.

}

@Override // XAResource
public int prepare(Xid xid) throws XAException {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is called after the start/end sequence has occurred. It signifies the start of the two-phase completion process. It is only called in the case of a true two-phase situation, i.e. where multiple resource managers are involved.

}

@Override // XAResource
public void commit(Xid xid, boolean onePhase) throws XAException {
Copy link
Member Author

@ljnelson ljnelson Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is called by the transaction manager during the two-phase completion stage, after the start/end sequence has occurred, and possibly after a prepare invocation (if onePhase is false). At this point the resource manager (represented by the Connection in our case) must commit its work, or the XA protocol will be deemed to have been violated.

If the onePhase argument is true, then prepare will not have been invoked.

Strictly speaking, this method may be invoked on any thread at any time after the conclusion of the start/end cycle.

}

@Override // XAResource
public void rollback(Xid xid) throws XAException {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is invoked as part of the two-phase completion process (after the start/end cycle, and possibly after a prepare invocation). Strictly speaking this may be invoked on any thread at any time after the start/end cycle. The resource manager must roll its work back.

}

@Override // XAResource
public Xid[] recover(int flags) throws XAException {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this method may be invoked by any thread at any time at the discretion of the transaction manager to recover from a failure with the two-phase completion process (so something going wrong after the start/end cycle). It is sort of irrelevant as this "local" XAResource implementation can't recover things anyway.

}

@Override // XAResource
public boolean isSameRM(XAResource xaResource) throws XAException {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is called when the transaction manager is trying to determine whether an XAResource in the process of being enlisted via Transaction#enlistResource(XAResource) is the "same as" another already-enlisted one. Over the decades all major XAResource implementors tend to coalesce around implementing this as an identity comparison. It's conceivable that you really could compare connectivity information (is my Connection a connection to the same host and port with the same credentials? what if the hostnames are different but resolve to the same IP? etc.), but there always seem to be loopholes—which is why almost all implementors choose an identity comparison.

Interestingly in Narayana this check is not used to see if a given XAResource is already registered, so again despite what the specification says imprecisely in this regard, simple equality is used first.

}

@Override // XAResource
public boolean setTransactionTimeout(int transactionTimeoutInSeconds) {
Copy link
Member Author

@ljnelson ljnelson Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting point of note: this is the first method that is called by the transaction manager (at least in Narayana's implementation), even before start. Returning false indicates that the XAResource implementation doesn't do anything with this information.

}
}

private XAException convert(XARoutine xaRoutine, Throwable e) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading many bug reports from different decades it's clear that no one, not even the specification authors, has made the error requirements clear to implementors, so this method plus the ExceptionConverter interface should allow an escape hatch for us if we end up getting the default behavior wrong.


// (Used via method reference only when an exceptionConverter was not supplied at construction time. This is the
// default implementation.)
private static XAException convert0(XARoutine xaRoutine, Exception e) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This represents the default XA exception conversion logic.

if (sqlState != null
&& (sqlState.startsWith("080")
|| sqlState.equalsIgnoreCase("08S01") // ("ess" not "five")
|| sqlState.equalsIgnoreCase("JZ006"))) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQLStates are extremely specific: 08 indicates a class of error that designates some kind of a connection error, and the other specific codes cited here are common issues that places like StackOverflow reveal are problematic in the real world.

}

// (Remapping BiFunction. Used in end() above.)
private static Association activeToIdle(Xid x, Association a) {
Copy link
Member Author

@ljnelson ljnelson Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers reading sequentially: you'll find all this "active" and "idle" talk and so on confusing unless you scroll to the bottom of this file and read a bit about the arcane branch states and branch association states defined by the specification.

if (LOGGER.isLoggable(Level.FINER)) {
LOGGER.entering(this.getClass().getName(), "start", new Object[] {x, a});
}
assert x != null; // x has already been vetted and is known to be non-null
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use the assert statement in this file carefully. Please let me keep it. Please.

*/


static record Association(BranchState branchState,
Copy link
Member Author

@ljnelson ljnelson Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holds a Connection together with a branch state and information needed to restore its initial state. Associations are immutable and logically move through a state machine defined by the XA specification.

if (!this.suspended()) {
switch (this.branchState()) {
case ACTIVE:
// OK; end(*) was called and didn't fail with an XAER_RB* code and we are not suspended
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll see notation like end(*): that's shorthand for end(<any flags>), i.e. end was called with any possible legal flag, i.e. TMSUCCESS or what have you.

this.xid(),
false,
this.connection(),
this.priorAutoCommit());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(We transition an association by creating a new one in the new state.)

// OK; end(*) was called and didn't fail with an XAER_RB* code and we are not suspended
//
// Associated -> Associated (T1 -> T1; unchanged)
// Active -> Idle (S1 -> S2)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These rules are drawn from the XA specification's state machine tables.

private static final Xid[] EMPTY_XID_ARRAY = new Xid[0];

// package-protected for testing only.
static final ConcurrentMap<Xid, Association> ASSOCIATIONS = new ConcurrentHashMap<>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're reading sequentially, this may not make sense, but you can come back to it later.

This XAResource implementation properly supports suspension and resuming of Connections' participation in global transactions. Because these operations can occur on any transaction manager thread, this Map is a ConcurrentMap. Because they do not in fact involve any state at all connected with the LocalXAResource instance itself, this Map can be static.

The Association class is an inner record found at the bottom of this file.

/**
* Creates a new {@link LocalXAResource}.
*
* @param connectionFunction a {@link Function} that accepts a {@link Xid} (supplied by the {@link #start(Xid, int)}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically a connection doesn't have to be supplied at construction time, because it won't be used unless start is called. So this implementation allows you to pass a Function that will be called by the start method, if and when it is called.

Copy link
Member

@tjquinno tjquinno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions/thoughts.



@Override // XAResource
public void start(Xid xid, int flags) throws XAException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming my understanding...

From the usage of flags in this code and the way flagsToString works, it seems that flags would more properly have been named (in the XAResource#start declaration) mode or something like that (or, as you pointed out, just use different methods that make very clear the intent and do away with flags entirely).

I usually think of "flags" as a group of (at least partially) orthogonal indicators of state or requested behavior. But it seems that's not what this is.

As I said, I'm not advising a change given that the interface declaration uses flags. Just making sure I'm understanding what I'm seeing.

Copy link
Member Author

@ljnelson ljnelson Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Most of the history of this extremely bizarre interface are lost to the mists of time but it is very clear it arose from the (C-centric) XA specification, where flags is indeed, well, flags: you do bitwise operations against them, they are a group of orthoganal indicators of state, etc. The exceedingly strange C-ish-sort-of-ported-to-Java documentation says: "flags - One of TMNOFLAGS, TMJOIN, or TMRESUME". Maybe they meant "after performing bitwise operations on the supplied int"? but they don't say that; in general the "specification" of these javax.transaction.xa.* classes is poor and full of mistakes. (Spot the fun typo in the documentation of XAException#errorCode that reflects the class' lineage!)

So they are int constants. There should, of course, have instead been a start method, a join method, a suspend method, a resume method, and so on and so on, but sadly this C-sort-of-ported-to-Java signature is what we have to work with.

You can see that I've tried to sort of map this to the various remapping functions (start, join, etc.) that have a BiFunction-compatible signature. But of course I have to implement the interface I've been given, warts and all.

.initCause(new IllegalArgumentException("xid: " + xid + "; flags: " + flagsToString(flags)));
}

this.computeAssociation(XARoutine.START, xid, remappingFunction);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand the role of remappingFunction. What has been "mapped" that will be in need of "remapping"? Is it the Association that is being remapped as a result of the about-to-happen state transition?

If so I'm not sure I have a good alternative for the name. It just seemed like a mild mismatch between the variable name and the function names being assigned to it but not a big deal.

Copy link
Member

@tjquinno tjquinno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The notes were a nice guided tour. LGTM.

@ljnelson ljnelson merged commit a703190 into helidon-io:helidon-3.x Dec 23, 2022
fawadali1 pushed a commit to fawadali1/helidon that referenced this pull request Jan 11, 2023
…lidon-io#5733)

Signed-off-by: Laird Nelson <[email protected]>
Merge pull request helidon-io#81 from NoodlesNZ/master

Add Github deploy
ljnelson added a commit to ljnelson/helidon that referenced this pull request Aug 23, 2023
ljnelson added a commit that referenced this pull request Aug 25, 2023
* Improves integrations/jdbc/jdbc to better support future JPA improvements; initial work (#5654)

Signed-off-by: Laird Nelson <[email protected]>

* Squashable commit; initial work (#5716)

Lays some groundwork with deprecation and cleanup and isolated improvements to support ongoing JPA improvements.

Signed-off-by: Laird Nelson <[email protected]>

* Introduces LocalXAResource and a few support classes in jta/jdbc. (#5733)

Signed-off-by: Laird Nelson <[email protected]>

* Adds connection unwrapping abilities to CDISEPlatform.java (#5790)

Signed-off-by: Laird Nelson <[email protected]>

* Introduces JtaConnection.java (#5905)

Signed-off-by: Laird Nelson <[email protected]>

* Fixes erroneous closing behavior in JtaConnection.java (#6321)

* Fixes erroneous closing behavior in JtaConnection.java

Signed-off-by: Laird Nelson <[email protected]>

* Minor JPA cleanups; part of overall refactoring effort (#6435)

Signed-off-by: Laird Nelson <[email protected]>

* Improving JPA pom.xml as part of overall JPA refactoring (#6508)

Signed-off-by: Laird Nelson <[email protected]>

* Fixes merge conflicts etc. from cherry-pick of c9a849e

Signed-off-by: Laird Nelson <[email protected]>

* Adds an enabled flag to JpaExtension to permit subsequent refactoring and replacement (#6512)

Adds an enabled flag to JpaExtension to permit subsequent refactoring and replacement

Signed-off-by: Laird Nelson <[email protected]>

* Adds more classes as part of overall JPA refactoring effort (#6584)

Signed-off-by: Laird Nelson <[email protected]>

* Lets unit tests validating JpaExtension and unit tests validating PersistenceExtension run side-by-side; continuation of overall fix for nested transaction problems (#7118)

* Lets unit tests validating JpaExtension and unit tests validating PersistenceExtension run side-by-side; continuation of overall fix for nested transaction problems

Signed-off-by: Laird Nelson <[email protected]>

* Resolves issue 7316, which features some intermittent database-related tests (#7317)

Signed-off-by: Laird Nelson <[email protected]>

* Addresses copyright plugin complaints after lots of cherry-picking from old 3.x commits

Signed-off-by: Laird Nelson <[email protected]>

---------

Signed-off-by: Laird Nelson <[email protected]>
dalexandrov pushed a commit to dalexandrov/helidon that referenced this pull request Aug 26, 2023
* Improves integrations/jdbc/jdbc to better support future JPA improvements; initial work (helidon-io#5654)

Signed-off-by: Laird Nelson <[email protected]>

* Squashable commit; initial work (helidon-io#5716)

Lays some groundwork with deprecation and cleanup and isolated improvements to support ongoing JPA improvements.

Signed-off-by: Laird Nelson <[email protected]>

* Introduces LocalXAResource and a few support classes in jta/jdbc. (helidon-io#5733)

Signed-off-by: Laird Nelson <[email protected]>

* Adds connection unwrapping abilities to CDISEPlatform.java (helidon-io#5790)

Signed-off-by: Laird Nelson <[email protected]>

* Introduces JtaConnection.java (helidon-io#5905)

Signed-off-by: Laird Nelson <[email protected]>

* Fixes erroneous closing behavior in JtaConnection.java (helidon-io#6321)

* Fixes erroneous closing behavior in JtaConnection.java

Signed-off-by: Laird Nelson <[email protected]>

* Minor JPA cleanups; part of overall refactoring effort (helidon-io#6435)

Signed-off-by: Laird Nelson <[email protected]>

* Improving JPA pom.xml as part of overall JPA refactoring (helidon-io#6508)

Signed-off-by: Laird Nelson <[email protected]>

* Fixes merge conflicts etc. from cherry-pick of c9a849e

Signed-off-by: Laird Nelson <[email protected]>

* Adds an enabled flag to JpaExtension to permit subsequent refactoring and replacement (helidon-io#6512)

Adds an enabled flag to JpaExtension to permit subsequent refactoring and replacement

Signed-off-by: Laird Nelson <[email protected]>

* Adds more classes as part of overall JPA refactoring effort (helidon-io#6584)

Signed-off-by: Laird Nelson <[email protected]>

* Lets unit tests validating JpaExtension and unit tests validating PersistenceExtension run side-by-side; continuation of overall fix for nested transaction problems (helidon-io#7118)

* Lets unit tests validating JpaExtension and unit tests validating PersistenceExtension run side-by-side; continuation of overall fix for nested transaction problems

Signed-off-by: Laird Nelson <[email protected]>

* Resolves issue 7316, which features some intermittent database-related tests (helidon-io#7317)

Signed-off-by: Laird Nelson <[email protected]>

* Addresses copyright plugin complaints after lots of cherry-picking from old 3.x commits

Signed-off-by: Laird Nelson <[email protected]>

---------

Signed-off-by: Laird Nelson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues for 3.x version branch jpa/jta OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants