From 30d98c1da2a32f719302668f8deb6ef4f371749c Mon Sep 17 00:00:00 2001
From: Robert Varga <rovarga@cisco.com>
Date: Thu, 24 Mar 2016 19:59:49 +0100
Subject: [PATCH] Enlarge critical section to cover processNextTransaction()

As it turns out the critical section is not sufficient to cover the case
when the user thread performs a submit/allocate/submit in the time window
between us releasing the in-flight transaction and taking the lock: we would
have to re-check inflightTx after taking the lock.

Since we are going to take the lock anyway, reverse the order of operations
by making processNextTransaction() synchronized, which means the user
thread will not be able to submit the transaction even when it observes
inflightTx as null outside the lock.

Change-Id: I688ceb5e8aae28f5e582b64e6bbaa64c9699c7f5
Signed-off-by: Robert Varga <rovarga@cisco.com>
---
 .../broker/impl/PingPongTransactionChain.java | 39 ++++++++++++++-----
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/PingPongTransactionChain.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/PingPongTransactionChain.java
index 26b5685094..b792826ec4 100644
--- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/PingPongTransactionChain.java
+++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/PingPongTransactionChain.java
@@ -207,19 +207,38 @@ public final class PingPongTransactionChain implements DOMTransactionChain {
         });
     }
 
-    private void processNextTransaction(final PingPongTransaction tx) {
+    /*
+     * We got invoked from the data store thread. We need to do two things:
+     * 1) release the in-flight transaction
+     * 2) process the potential next transaction
+     *
+     * We have to perform 2) under lock. We could perform 1) without locking, but that means the CAS result may
+     * not be accurate, as a user thread may submit the ready transaction before we acquire the lock -- and checking
+     * for next transaction is not enough, as that may have also be allocated (as a result of a quick
+     * submit/allocate/submit between 1) and 2)). Hence we'd end up doing the following:
+     * 1) CAS of inflightTx
+     * 2) take lock
+     * 3) volatile read of inflightTx
+     *
+     * Rather than doing that, we keep this method synchronized, hence performing only:
+     * 1) take lock
+     * 2) CAS of inflightTx
+     *
+     * Since the user thread is barred from submitting the transaction (in processIfReady), we can then proceed with
+     * the knowledge that inflightTx is null -- processTransaction() will still do a CAS, but that is only for
+     * correctness.
+     */
+    private synchronized void processNextTransaction(final PingPongTransaction tx) {
         final boolean success = INFLIGHT_UPDATER.compareAndSet(this, tx, null);
         Preconditions.checkState(success, "Completed transaction %s while %s was submitted", tx, inflightTx);
 
-        synchronized (this) {
-            final PingPongTransaction nextTx = READY_UPDATER.getAndSet(this, null);
-            if (nextTx != null) {
-                processTransaction(nextTx);
-            } else if (shutdownTx != null) {
-                processTransaction(shutdownTx);
-                delegate.close();
-                shutdownTx = null;
-            }
+        final PingPongTransaction nextTx = READY_UPDATER.getAndSet(this, null);
+        if (nextTx != null) {
+            processTransaction(nextTx);
+        } else if (shutdownTx != null) {
+            processTransaction(shutdownTx);
+            delegate.close();
+            shutdownTx = null;
         }
     }
 
-- 
2.36.6