]
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.