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

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.

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

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

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.