[JBoss JIRA] (JBTM-2846) Thread-safety problem in BasicAction
by Michael Musgrove (JIRA)
[ https://issues.jboss.org/browse/JBTM-2846?page=com.atlassian.jira.plugin.... ]
Michael Musgrove commented on JBTM-2846:
----------------------------------------
I do not see how the scenario you suggest can occur if we protect the update of actionStatus in BasicAction#preventCommit is protected by the instance monitor:
synchronized (this) {
if (actionStatus == ActionStatus.RUNNING)
actionStatus = ActionStatus.ABORT_ONLY;
}
> Thread-safety problem in BasicAction
> ------------------------------------
>
> Key: JBTM-2846
> URL: https://issues.jboss.org/browse/JBTM-2846
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Components: Transaction Core
> Affects Versions: 5.4.0.Final
> Reporter: David Lloyd
> Assignee: Michael Musgrove
> Priority: Critical
>
> It is possible for multiple threads to have a given transaction association at the same time ({{com.arjuna.ats.arjuna.coordinator.BasicAction#_childThreads}}). However, only inconsistent protection is provided for {{com.arjuna.ats.arjuna.coordinator.BasicAction#actionStatus}}. In particular, the {{com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionImple#setRollbackOnly}} method directly calls {{com.arjuna.ats.arjuna.coordinator.BasicAction#preventCommit}}, which, without lock or atomicity, both reads and modifies {{actionStatus}}. This will result in a poorly-defined outcome when threads concurrently call {{setRollbackOnly()}} and, say, {{rollback()}}.
> Other methods access {{actionStatus}} without regard to concurrently protection, including:
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#save_state}}
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#toString}}
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#restore_state}}
> Some methods appear to be assuming that other methods will acquire the instance monitor before calling them; as a matter of best practice, such methods should have {{assert Thread.holdsLock(this);}} at their beginning point.
> Other fields in the class should also be analyzed for unsafe access.
--
This message was sent by Atlassian JIRA
(v7.2.3#72005)
7 years, 1 month
[JBoss JIRA] (JBTM-2850) Call xa_end on duplicate XAResource as per JTA 1.2 specification
by Tom Jenkinson (JIRA)
[ https://issues.jboss.org/browse/JBTM-2850?page=com.atlassian.jira.plugin.... ]
Issue was automatically transitioned when Tom Jenkinson created pull request #1135 in GitHub
--------------------------------------------------------------------------------------------
Status: Pull Request Sent (was: Open)
> Call xa_end on duplicate XAResource as per JTA 1.2 specification
> ----------------------------------------------------------------
>
> Key: JBTM-2850
> URL: https://issues.jboss.org/browse/JBTM-2850
> Project: JBoss Transaction Manager
> Issue Type: Enhancement
> Components: JTA
> Reporter: Tom Jenkinson
> Assignee: Tom Jenkinson
> Fix For: 5.next
>
>
> JTA 1.2 changed requirement:
> "A transaction manager is, however, required to implicitly ensure the association of any associated XAResource is ended, via the appropriate XAResource.end call, immediately prior to completion;"
> The change is that it no longer is confined to any associated ** resource **, but now specifies any associated ** XAResource **
> What is happening at the moment for two difference instance of an XAR but where isSameRM is true:
> Resource1 start TMNOFLAGS
> DuplicateResource1 start TMJOIN
> Resource2 start TMNOFLAGS
> Resource1 end TMSUCCESS
> Resource1 prepare
> Resource2 end TMSUCCESS
> Resource2 prepare
> Resource1 commit
> Resource2 commit
> Post https://java.net/jira/browse/JTA_SPEC-3 this should be:
> Resource1 start TMNOFLAGS
> DuplicateResource1 start TMJOIN
> Resource2 start TMNOFLAGS
> Resource1 end TMSUCCESS
> DuplicateResource1 end TMSUCCESS
> Resource1 prepare
> Resource2 end TMSUCCESS
> Resource2 prepare
> Resource1 commit
> Resource2 commit
--
This message was sent by Atlassian JIRA
(v7.2.3#72005)
7 years, 1 month
[JBoss JIRA] (JBTM-2846) Thread-safety problem in BasicAction
by David Lloyd (JIRA)
[ https://issues.jboss.org/browse/JBTM-2846?page=com.atlassian.jira.plugin.... ]
David Lloyd commented on JBTM-2846:
-----------------------------------
OK I have good news and bad news. Bad news first: there's a real (non-theoretical) thread safety problem that may crop up under load which is not solved solely by setting {{volatile}}. It happens under the described circumstances, where setRollbackOnly() can return successfully yet prepare() or commit(true) also succeeds.
The good news is I worked up a quick patch for it: https://github.com/jbosstm/narayana/compare/master...dmlloyd:jbtm-2846
If it looks OK I can put it in as a PR. I tried to explain the logic in the comments so hopefully it's clear why it's needed and why it works. But I can't test it locally through the Narayana test suite at the moment due to other things I have going on, so it's up to you guys what you want to do.
> Thread-safety problem in BasicAction
> ------------------------------------
>
> Key: JBTM-2846
> URL: https://issues.jboss.org/browse/JBTM-2846
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Components: Transaction Core
> Affects Versions: 5.4.0.Final
> Reporter: David Lloyd
> Assignee: Michael Musgrove
> Priority: Critical
>
> It is possible for multiple threads to have a given transaction association at the same time ({{com.arjuna.ats.arjuna.coordinator.BasicAction#_childThreads}}). However, only inconsistent protection is provided for {{com.arjuna.ats.arjuna.coordinator.BasicAction#actionStatus}}. In particular, the {{com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionImple#setRollbackOnly}} method directly calls {{com.arjuna.ats.arjuna.coordinator.BasicAction#preventCommit}}, which, without lock or atomicity, both reads and modifies {{actionStatus}}. This will result in a poorly-defined outcome when threads concurrently call {{setRollbackOnly()}} and, say, {{rollback()}}.
> Other methods access {{actionStatus}} without regard to concurrently protection, including:
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#save_state}}
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#toString}}
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#restore_state}}
> Some methods appear to be assuming that other methods will acquire the instance monitor before calling them; as a matter of best practice, such methods should have {{assert Thread.holdsLock(this);}} at their beginning point.
> Other fields in the class should also be analyzed for unsafe access.
--
This message was sent by Atlassian JIRA
(v7.2.3#72005)
7 years, 1 month
[JBoss JIRA] (JBTM-2854) XATerminatorImple.importTransaction can produce ArrayIndexOutOfBoundsException
by Michael Musgrove (JIRA)
[ https://issues.jboss.org/browse/JBTM-2854?page=com.atlassian.jira.plugin.... ]
Michael Musgrove reassigned JBTM-2854:
--------------------------------------
Assignee: Tom Jenkinson (was: Michael Musgrove)
> XATerminatorImple.importTransaction can produce ArrayIndexOutOfBoundsException
> ------------------------------------------------------------------------------
>
> Key: JBTM-2854
> URL: https://issues.jboss.org/browse/JBTM-2854
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Components: JTA
> Reporter: David Lloyd
> Assignee: Tom Jenkinson
>
> Here is what the stack trace looks like:
> {noformat}
> Caused by: Remote exception java.lang.ArrayIndexOutOfBoundsException: 32
> at com.arjuna.ats.jta.xa.XATxConverter.getSubordinateNodeName(XATxConverter.java:204)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.subordinate.jca.SubordinateAtomicAction.<init>(SubordinateAtomicAction.java:115)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.subordinate.jca.TransactionImple.<init>(TransactionImple.java:57)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.jca.TransactionImporterImple.addImportedTransaction(TransactionImporterImple.java:281)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.jca.TransactionImporterImple.importRemoteTransaction(TransactionImporterImple.java:105)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.jca.XATerminatorImple.importTransaction(XATerminatorImple.java:599)
> at org.wildfly.transaction.client.provider.jboss.JBossLocalTransactionProvider$XAImporterImpl.findOrImportTransaction(JBossLocalTransactionProvider.java:585)
> ... 11 more
> {noformat}
> It appears that there is no protection against an XID of inadequate length in XATxConverter#setSubordinateNodeName.
> At present the protocol is attempting to import an XID which contains only the gtid from the master node (based on what I discussed with [~tomjenkinson] some time ago at a previous meeting). This can be changed if it is wrong, but either way this method should fail with a friendlier exception if the XID is not valid for some reason.
--
This message was sent by Atlassian JIRA
(v7.2.3#72005)
7 years, 1 month
[JBoss JIRA] (JBTM-2856) Add a way to register an interposed synchronization without associating the txn
by David Lloyd (JIRA)
David Lloyd created JBTM-2856:
---------------------------------
Summary: Add a way to register an interposed synchronization without associating the txn
Key: JBTM-2856
URL: https://issues.jboss.org/browse/JBTM-2856
Project: JBoss Transaction Manager
Issue Type: Enhancement
Components: Application Server Integration
Reporter: David Lloyd
Assignee: Amos Feng
It would be very useful to the application server if there were an API method to register an interposed synchronization on a transaction that was not currently associated with the calling thread, the same way that non-interposed synchronizations can be. Right now the only two options are to use TransactionSynchronizationRegistry, which requires that the existing transaction be suspended and a new one associated, or to use reflection to directly call the registerSynchronizationImple method of the transaction. Neither one is optimal.
--
This message was sent by Atlassian JIRA
(v7.2.3#72005)
7 years, 1 month
[JBoss JIRA] (JBTM-2846) Thread-safety problem in BasicAction
by Michael Musgrove (JIRA)
[ https://issues.jboss.org/browse/JBTM-2846?page=com.atlassian.jira.plugin.... ]
Issue was automatically transitioned when Michael Musgrove created pull request #1134 in GitHub
-----------------------------------------------------------------------------------------------
Status: Pull Request Sent (was: Open)
> Thread-safety problem in BasicAction
> ------------------------------------
>
> Key: JBTM-2846
> URL: https://issues.jboss.org/browse/JBTM-2846
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Components: Transaction Core
> Affects Versions: 5.4.0.Final
> Reporter: David Lloyd
> Assignee: Michael Musgrove
> Priority: Critical
>
> It is possible for multiple threads to have a given transaction association at the same time ({{com.arjuna.ats.arjuna.coordinator.BasicAction#_childThreads}}). However, only inconsistent protection is provided for {{com.arjuna.ats.arjuna.coordinator.BasicAction#actionStatus}}. In particular, the {{com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionImple#setRollbackOnly}} method directly calls {{com.arjuna.ats.arjuna.coordinator.BasicAction#preventCommit}}, which, without lock or atomicity, both reads and modifies {{actionStatus}}. This will result in a poorly-defined outcome when threads concurrently call {{setRollbackOnly()}} and, say, {{rollback()}}.
> Other methods access {{actionStatus}} without regard to concurrently protection, including:
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#save_state}}
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#toString}}
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#restore_state}}
> Some methods appear to be assuming that other methods will acquire the instance monitor before calling them; as a matter of best practice, such methods should have {{assert Thread.holdsLock(this);}} at their beginning point.
> Other fields in the class should also be analyzed for unsafe access.
--
This message was sent by Atlassian JIRA
(v7.2.3#72005)
7 years, 1 month
[JBoss JIRA] (JBTM-2855) Consider using assertions to validate locking assumptions
by Michael Musgrove (JIRA)
Michael Musgrove created JBTM-2855:
--------------------------------------
Summary: Consider using assertions to validate locking assumptions
Key: JBTM-2855
URL: https://issues.jboss.org/browse/JBTM-2855
Project: JBoss Transaction Manager
Issue Type: Enhancement
Components: Transaction Core
Affects Versions: 5.5.2.Final
Reporter: Michael Musgrove
Assignee: Michael Musgrove
Priority: Optional
Fix For: 5.later
Some methods in BasicAction assume that other methods will acquire monitors before calling them; it would aid both maintenance/readability and testing if we such methods have assert Thread.holdsLock(this); at their beginning point.
--
This message was sent by Atlassian JIRA
(v7.2.3#72005)
7 years, 1 month
[JBoss JIRA] (JBTM-2854) XATerminatorImple.importTransaction can produce ArrayIndexOutOfBoundsException
by Tom Jenkinson (JIRA)
[ https://issues.jboss.org/browse/JBTM-2854?page=com.atlassian.jira.plugin.... ]
Tom Jenkinson commented on JBTM-2854:
-------------------------------------
The root parent name is in the gtrid, but a subordinate parent name is in the bqual (cos we can't edit the gtrid).
Thanks!
> XATerminatorImple.importTransaction can produce ArrayIndexOutOfBoundsException
> ------------------------------------------------------------------------------
>
> Key: JBTM-2854
> URL: https://issues.jboss.org/browse/JBTM-2854
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Components: JTA
> Reporter: David Lloyd
> Assignee: Michael Musgrove
>
> Here is what the stack trace looks like:
> {noformat}
> Caused by: Remote exception java.lang.ArrayIndexOutOfBoundsException: 32
> at com.arjuna.ats.jta.xa.XATxConverter.getSubordinateNodeName(XATxConverter.java:204)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.subordinate.jca.SubordinateAtomicAction.<init>(SubordinateAtomicAction.java:115)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.subordinate.jca.TransactionImple.<init>(TransactionImple.java:57)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.jca.TransactionImporterImple.addImportedTransaction(TransactionImporterImple.java:281)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.jca.TransactionImporterImple.importRemoteTransaction(TransactionImporterImple.java:105)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.jca.XATerminatorImple.importTransaction(XATerminatorImple.java:599)
> at org.wildfly.transaction.client.provider.jboss.JBossLocalTransactionProvider$XAImporterImpl.findOrImportTransaction(JBossLocalTransactionProvider.java:585)
> ... 11 more
> {noformat}
> It appears that there is no protection against an XID of inadequate length in XATxConverter#setSubordinateNodeName.
> At present the protocol is attempting to import an XID which contains only the gtid from the master node (based on what I discussed with [~tomjenkinson] some time ago at a previous meeting). This can be changed if it is wrong, but either way this method should fail with a friendlier exception if the XID is not valid for some reason.
--
This message was sent by Atlassian JIRA
(v7.2.3#72005)
7 years, 1 month
[JBoss JIRA] (JBTM-2854) XATerminatorImple.importTransaction can produce ArrayIndexOutOfBoundsException
by David Lloyd (JIRA)
[ https://issues.jboss.org/browse/JBTM-2854?page=com.atlassian.jira.plugin.... ]
David Lloyd commented on JBTM-2854:
-----------------------------------
OK in that case I agree to lower the priority and I'll ensure that the subordinate is importing the bqual. I had thought the parent name was encoded in the gtid, but that was just an unwarranted assumption on my part.
> XATerminatorImple.importTransaction can produce ArrayIndexOutOfBoundsException
> ------------------------------------------------------------------------------
>
> Key: JBTM-2854
> URL: https://issues.jboss.org/browse/JBTM-2854
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Components: JTA
> Reporter: David Lloyd
> Assignee: Michael Musgrove
>
> Here is what the stack trace looks like:
> {noformat}
> Caused by: Remote exception java.lang.ArrayIndexOutOfBoundsException: 32
> at com.arjuna.ats.jta.xa.XATxConverter.getSubordinateNodeName(XATxConverter.java:204)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.subordinate.jca.SubordinateAtomicAction.<init>(SubordinateAtomicAction.java:115)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.subordinate.jca.TransactionImple.<init>(TransactionImple.java:57)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.jca.TransactionImporterImple.addImportedTransaction(TransactionImporterImple.java:281)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.jca.TransactionImporterImple.importRemoteTransaction(TransactionImporterImple.java:105)
> at com.arjuna.ats.internal.jta.transaction.arjunacore.jca.XATerminatorImple.importTransaction(XATerminatorImple.java:599)
> at org.wildfly.transaction.client.provider.jboss.JBossLocalTransactionProvider$XAImporterImpl.findOrImportTransaction(JBossLocalTransactionProvider.java:585)
> ... 11 more
> {noformat}
> It appears that there is no protection against an XID of inadequate length in XATxConverter#setSubordinateNodeName.
> At present the protocol is attempting to import an XID which contains only the gtid from the master node (based on what I discussed with [~tomjenkinson] some time ago at a previous meeting). This can be changed if it is wrong, but either way this method should fail with a friendlier exception if the XID is not valid for some reason.
--
This message was sent by Atlassian JIRA
(v7.2.3#72005)
7 years, 1 month
[JBoss JIRA] (JBTM-2846) Thread-safety problem in BasicAction
by David Lloyd (JIRA)
[ https://issues.jboss.org/browse/JBTM-2846?page=com.atlassian.jira.plugin.... ]
David Lloyd commented on JBTM-2846:
-----------------------------------
The reason I didn't include it is logical: we use test cases to find problems. But in this case, I've already found the problem. I don't need a test case to prove that it is a problem because the JMM proves it for me; it's a problem whether or not it's likely to be observed. And the way this logic runs, the more you consider unobservable problems to be non-problems, the more you're stacking the deck for a quite dark day.
> Thread-safety problem in BasicAction
> ------------------------------------
>
> Key: JBTM-2846
> URL: https://issues.jboss.org/browse/JBTM-2846
> Project: JBoss Transaction Manager
> Issue Type: Bug
> Components: Transaction Core
> Affects Versions: 5.4.0.Final
> Reporter: David Lloyd
> Assignee: Michael Musgrove
> Priority: Critical
>
> It is possible for multiple threads to have a given transaction association at the same time ({{com.arjuna.ats.arjuna.coordinator.BasicAction#_childThreads}}). However, only inconsistent protection is provided for {{com.arjuna.ats.arjuna.coordinator.BasicAction#actionStatus}}. In particular, the {{com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionImple#setRollbackOnly}} method directly calls {{com.arjuna.ats.arjuna.coordinator.BasicAction#preventCommit}}, which, without lock or atomicity, both reads and modifies {{actionStatus}}. This will result in a poorly-defined outcome when threads concurrently call {{setRollbackOnly()}} and, say, {{rollback()}}.
> Other methods access {{actionStatus}} without regard to concurrently protection, including:
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#save_state}}
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#toString}}
> * {{com.arjuna.ats.arjuna.coordinator.BasicAction#restore_state}}
> Some methods appear to be assuming that other methods will acquire the instance monitor before calling them; as a matter of best practice, such methods should have {{assert Thread.holdsLock(this);}} at their beginning point.
> Other fields in the class should also be analyzed for unsafe access.
--
This message was sent by Atlassian JIRA
(v7.2.3#72005)
7 years, 1 month