[jbossts-issues] [JBoss JIRA] (JBTM-2846) Thread-safety problem in BasicAction

[David Lloyd (JIRA)]

    [ https://issues.jboss.org/browse/JBTM-2846?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13369324#comment-13369324 ] 

David Lloyd commented on JBTM-2846:
-----------------------------------

You are right, but with this approach a different problem occurs. The lock which is held during state changes would prevent setRollbackOnly from working during subordinate synchronization execution.

This is an example of why synchronization should generally protect data, not code.

> 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)