Adjusting checks of completionBits from SubordinateTransactionControl #17

ochaloup · 2017-03-03T15:12:13Z

I think there is a wrong check in rollback method of JBossLocalTransactionProvider$Entry class.

What I can see from my testing and when looking to the code rollback method should change the if to (oldVal & BIT_PREPARE_OR_ROLLBACK) == 0 - meaning throw InvalidStateException when transaction is not in prepared state.

The other changes in this commit are not tested and they came only from my feeling. I would be happy if you can review this PR.

dmlloyd · 2017-03-03T15:18:35Z

src/main/java/org/wildfly/transaction/client/provider/jboss/JBossLocalTransactionProvider.java

@@ -323,7 +323,7 @@ void rollbackLocal() throws SystemException {
            int oldVal;
            do {
                oldVal = completionBits.get();
-                if ((oldVal & BIT_PREPARE_OR_ROLLBACK) != 0) {
+                if ((oldVal & BIT_PREPARE_OR_ROLLBACK) == 0) {


Normally this bit is set when the transaction is either PREPARED or ROLLED_BACK. So if we do a rollback() when the transaction was already PREPARED or ROLLED_BACK, then we throw the exception.

In other words, these bits represent what states we've already reached, so that we know what state is valid to come next.

Which is to say that '!=' should be correct here.

ok, I see. I'm still not fully sure if I follow :)

(oldVal & BIT_PREPARE_OR_ROLLBACK) != 0) says - if the current state is in prepare or rollback then throw an exception. But calling rollback could be from state where transaction was preapred. Ie. transaction.begin -> transation.prepare -> transaction.rollback. I mean when transaction is prepared is fine to rollback it, what I'm missing here?

I thought it was only valid to "forget" in this state. But if I'm wrong about that, then the entire check can be removed since either true or false are both OK.

All my other changes came from the feeling that rollbacking prepared transaction is fully ok. So I'm leaving other comments untouched till I understand how this behaves.

Btw. if this is ok then I need to find out fix on some different place and I need to think about it a little bit more.

dmlloyd · 2017-03-03T15:19:21Z

src/main/java/org/wildfly/transaction/client/provider/jboss/JBossLocalTransactionProvider.java

@@ -337,7 +337,7 @@ void commitLocal() throws HeuristicRollbackException, RollbackException, Heurist
            int oldVal;
            do {
                oldVal = completionBits.get();
-                if ((oldVal & BIT_PREPARE_OR_ROLLBACK) != 0 || (oldVal & BIT_COMMIT_OR_FORGET) != 0) {
+                if ((oldVal & BIT_PREPARE_OR_ROLLBACK) == 0 || (oldVal & BIT_COMMIT_OR_FORGET) != 0) {


In this case we want to keep '!=' because this essentially says, "If we have prepared or rolled back, or if we've committed or forgotten, throw the exception".

We can probably skip the check here (and keep the CAS) if we want to just let commit() fail on its own.

dmlloyd · 2017-03-03T15:19:50Z

src/main/java/org/wildfly/transaction/client/provider/jboss/JBossLocalTransactionProvider.java

@@ -361,7 +361,7 @@ public void rollback() throws XAException {
            int oldVal;
            do {
                oldVal = completionBits.get();
-                if ((oldVal & BIT_PREPARE_OR_ROLLBACK) != 0) {
+                if ((oldVal & BIT_PREPARE_OR_ROLLBACK) == 0) {


In this case we keep the '!=' because we want to throw an exception if rollback() was called after prepare() or rollback().

dmlloyd · 2017-03-03T15:27:10Z

src/main/java/org/wildfly/transaction/client/provider/jboss/JBossLocalTransactionProvider.java

@@ -509,7 +509,7 @@ public void commit(final boolean onePhase) throws XAException {
            int oldVal;
            do {
                oldVal = completionBits.get();
-                if (onePhase && (oldVal & BIT_PREPARE_OR_ROLLBACK) != 0 || (oldVal & BIT_COMMIT_OR_FORGET) != 0) {
+                if ((onePhase && (oldVal & BIT_PREPARE_OR_ROLLBACK) != 0) || (oldVal & BIT_COMMIT_OR_FORGET) != 0 || (oldVal & BIT_PREPARE_OR_ROLLBACK) == 0) {


This one is almost correct, I think, ignoring the useless extra ().

Transliterating:

if:
onePhase and we've already prepared/rolled back
or:
we've already committed
or:
we have not yet prepared

But the last check should be "or: !onePhase and we have not yet prepared". Otherwise, yes I think you found a real mistake in this one.

Since onePhase is compared in one sense in half the expression and the opposite sense in the other half, it's probably best written like this:

if ((oldVal & BIT_COMMIT_OR_FORGET) != 0 && onePhase ? (oldValue & BIT_PREPARE_OR_ROLLBACK) != 0 : (oldValue & BIT_PREPARE_OR_ROLLBACK) == 0) {

Since the sense of the latter flag check varies in relation to onePhase, it can be further simplified:

if ((oldVal & BIT_COMMIT_OR_FORGET) != 0 && onePhase == ((oldValue & BIT_PREPARE_OR_ROLLBACK) != 0)) {

Which transliterates as: "If we already committed/forgot, or whether we prepared is the same as onePhase, throw exception".

dmlloyd · 2017-03-03T15:49:07Z

Given our conversation, I think that maybe all these checks are wrong and we can just remove completionBits completely.

ochaloup · 2017-03-06T06:52:43Z

@dmlloyd ok, from my insight I think it could be fine to remove them. But maybe a check is worthy at some places. I mean for example is invalid to call commit when there is no onephase optimization and there was no prepare called.

As I'm not all sure here but Narayana handles situations of execution in wrong context. As the call is delegated to it, the particular XAException should be called from it. But I understand that these checks could be aimed for different TM providers too.

To summarize that somehow - are you ok for me change this PR to remove all the usage of completionBits from the JBossLocalTransactionProvider$Entry and leave the calls unchecked at the wfly txn client layer?

Thanks

ochaloup · 2017-03-07T15:48:31Z

@dmlloyd per our discussion I removed the check bits code completely. Checked with inflow transaction processing and that works.

This was agreed that is not needed anymore at this time. The checks, as they were defined, causes troubles on jca inflow transction processing (JBEAP-8993)

ochaloup · 2017-03-08T12:55:11Z

@dmlloyd what do you think, could this be part of 1.0.0.Beta19 release? You know I'm asking as my fix WFLY-8275 depends on it 😁

dmlloyd reviewed Mar 3, 2017

View reviewed changes

ochaloup mentioned this pull request Mar 6, 2017

[WFLY-8275][JBEAP-8993] jca inflow integration with wildfly transaction client jbosstm/narayana#1144

Merged

ochaloup force-pushed the rollback-bit-check branch from b8c8b6e to 4e30439 Compare March 7, 2017 15:47

Removing completionBits from SubordinateTransactionControl

65ef0e2

This was agreed that is not needed anymore at this time. The checks, as they were defined, causes troubles on jca inflow transction processing (JBEAP-8993)

ochaloup force-pushed the rollback-bit-check branch from 4e30439 to 65ef0e2 Compare March 7, 2017 15:49

dmlloyd merged commit 0fe21dd into wildfly:master Mar 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjusting checks of completionBits from SubordinateTransactionControl #17

Adjusting checks of completionBits from SubordinateTransactionControl #17

ochaloup commented Mar 3, 2017

dmlloyd Mar 3, 2017

dmlloyd Mar 3, 2017

ochaloup Mar 3, 2017

dmlloyd Mar 3, 2017

ochaloup Mar 3, 2017

dmlloyd Mar 3, 2017

dmlloyd Mar 3, 2017

dmlloyd Mar 3, 2017

dmlloyd Mar 3, 2017

dmlloyd commented Mar 3, 2017

ochaloup commented Mar 6, 2017

ochaloup commented Mar 7, 2017

ochaloup commented Mar 8, 2017 •

edited

Loading

Adjusting checks of completionBits from SubordinateTransactionControl #17

Adjusting checks of completionBits from SubordinateTransactionControl #17

Conversation

ochaloup commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmlloyd commented Mar 3, 2017

ochaloup commented Mar 6, 2017

ochaloup commented Mar 7, 2017

ochaloup commented Mar 8, 2017 • edited Loading

ochaloup commented Mar 8, 2017 •

edited

Loading