Could AtomicTransaction use nondeterministic locking?

[ochaloup]

I got few troubles coming from Coverity code static analysis tools and one of them is pointing out that AtomicTransaction [5] class could have nondeterministic locking behavior.

The issue relates to class and methods
  * begin
  * suspend
  * rollback
  * commit

lock_acquire: Acquiring lock `AtomicTransaction._theStatus`

Bad choice of lock object (BAD_LOCK_OBJECT)3. lock_on_assigned_field:  Locking on the object referenced by field `_theStatus`. This lock  acquisition may race with another thread assigning to this field. The  contents of `_theStatus` may change while a thread is inside the  critical section, allowing two threads to enter the critical section  simultaneously.
   Instead of using `_theStatus` as a lock, create a final field of type Object which is only used as a lock.
assign_to_field: The expression `_theStatus = current.get_status()`  assigns a new value to `_theStatus`, a field whose contents are used as a  lock. The locking behavior of this function may allow this assignment  to occur multiple times.

To recap - the issue is caused by fact that `_theStatus` is reassigned to different instance which could cause wrong locking

Could be this a real issue or the behavior is intentional?

Thank you

Ondra

[5] https://github.com/jbosstm/narayana/blob/master/ArjunaJTS/jts/classes/com/arjuna/ats/jts/extensions/AtomicTransaction.java

[marklittle]

What's the example/test that throws this up?

[marklittle]

I can't see a good reason it couldn't be changed as you suggest, but would like to see the example/test to be sure.

[ochaloup]

This does not come from a test or example which fails. The warning is from static code analysis tool as FindBugs is. It points out at these lines of code.

That's why I came with it here to be considered whether it could be a problem or it's just a a false-positive report.

[tomjenkinson]

Looking at the code I am not sure if there is any possibility of a bug given the context.

It looks like _theStatus is used to guard access to _theAction. _theAction is set in two places suspend and begin. The code in _begin is written in such a manner as it seems impossible for a wrong read on _theAction (especially given the suspend checking for null _theAction).

However, looking at the code itself I wonder if there is a performance improvement that could be made here? It looks like begin() will synchronize all threads beginning a transaction  [1] on Status.NoTransaction [2]. The following snippet will show what I mean:

@Test
public void testConcurrentBegin () throws Exception

{

  CountDownLatch latch = new CountDownLatch(2);
  new Thread( new Runnable() {

   public void run() {

   try {

  AtomicTransaction A = new AtomicTransaction();
   A.begin();
   A.rollback();
   } catch (SubtransactionsUnavailable | NoTransaction | WrongTransaction e) {

   fail(e.getMessage());
   } finally {

   latch.countDown();
   }

  }

  }).start();

  new Thread( new Runnable() {

   public void run() {

   try {

  AtomicTransaction B = new AtomicTransaction();
   B.begin();
   B.rollback();
   } catch (SubtransactionsUnavailable | NoTransaction | WrongTransaction e) {

   fail(e.getMessage());
   } finally {

   latch.countDown();
   }

  }

  }).start();
   latch.await();
}

If you put a debugger breakpoint at [3] then run that test you will see only one thread is allowed through.

[1]

narayana/AtomicTransaction.java at master · jbosstm/narayana · GitHub

[2]

narayana/AtomicTransaction.java at master · jbosstm/narayana · GitHub

[3]

narayana/AtomicTransaction.java at master · jbosstm/narayana · GitHub

[ochaloup]

on static code analysis problem: I see, thank you! Then this report could be considered as intentional and as the main thing correct behavior.

Just for the sake of completeness it could not happen that _theAction = current.getControlWrapper() would produces null? In suspend method _theAction = OTSImpleManager.current().suspendWrapper() seems could produce null. In such case it still can't be a root of potential issue?

[tomjenkinson]

Hi Ondra,

getControlWrapper returns _theManager.current() so we need to find a way for that to be null.

Looking at begin there are the following scenarios:

1. _theManager.current() was not null in which case we try to create a sub_transaction and throw an exception if its not possible

2. _theManager.current() was null in which case we initialize a new control wrapper [1] or [2] and push it [3] in a way that _theManager.current() could never return null from after: push [4] peek [5]

3. Interceptor pop/purge: ContextServerRequestinterceptor [6] InterpositionServerRequestInterceptor [7]

3 should likely explain why the null check is needed in suspend. I can't see how that would be called in the begin code path so it seems safe to me to not check for null. I see some Javadocs about "This interception point shall execute in the same thread as the target invocation" so I don't think it could happen between the two lines.

As I don't currently have a performance report to suggest that making the change on performance grounds would yield any great benefit I don't propose making the change yet but if you have some numbers on how it would be beneficial we can certainly look to make the change.

Thanks for the report,

Tom

[1] narayana/CurrentImple.java at master · jbosstm/narayana · GitHub

[2] narayana/CurrentImple.java at master · jbosstm/narayana · GitHub

[3] narayana/CurrentImple.java at master · jbosstm/narayana · GitHub

[4] narayana/ContextManager.java at master · jbosstm/narayana · GitHub

[5] narayana/ContextManager.java at master · jbosstm/narayana · GitHub

[6] narayana/ContextServerRequestInterceptorImpl.java at master · jbosstm/narayana · GitHub

[7] narayana/InterpositionServerRequestInterceptorImpl.java at master · jbosstm/narayana · GitHub

[marklittle]

Just to be sure everyone realises this: it is legal (in the standard) to have a null Control/ControlWrapper and legal to pass null to resume.