From 91b351d9bca0c0621a464568d815cbe491dec856 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Mon, 8 Jun 2015 20:18:31 +0200 Subject: [PATCH] BUG-3516: make PingPongTransactionChain.close() asynchronous When the system is critical loads Thread.yield can bring a long-term blocking effect for hijacked threads like Netty. We should not be blocking for prolonged time. Rework the shutdown logic to be asynchronous, and scheduling the potential outstanding transaction to complete as appropriate. Also fixes the case where we would end up not reporting a transaction failure if the transaction is readied, but was not submitted to the backend. Change-Id: Ic7796a980d9e87242f70b7f7b9cdb30caeab9dd9 Signed-off-by: Vaclav Demcak Signed-off-by: Robert Varga --- .../broker/impl/PingPongTransactionChain.java | 61 ++++++++++++------- 1 file changed, 39 insertions(+), 22 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 20e8422800..26b5685094 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 @@ -57,6 +57,8 @@ public final class PingPongTransactionChain implements DOMTransactionChain { @GuardedBy("this") private boolean failed; + @GuardedBy("this") + private PingPongTransaction shutdownTx; /** * This updater is used to manipulate the "ready" transaction. We perform only atomic @@ -124,6 +126,8 @@ public final class PingPongTransactionChain implements DOMTransactionChain { } private synchronized PingPongTransaction slowAllocateTransaction() { + Preconditions.checkState(shutdownTx == null, "Transaction chain %s has been shut down", this); + final DOMDataReadWriteTransaction delegateTx = delegate.newReadWriteTransaction(); final PingPongTransaction newTx = new PingPongTransaction(delegateTx); @@ -203,27 +207,34 @@ public final class PingPongTransactionChain implements DOMTransactionChain { }); } - private void transactionSuccessful(final PingPongTransaction tx, final Void result) { - LOG.debug("Transaction {} completed successfully", tx); - + private void processNextTransaction(final PingPongTransaction tx) { final boolean success = INFLIGHT_UPDATER.compareAndSet(this, tx, null); - Preconditions.checkState(success, "Successful transaction %s while %s was submitted", tx, inflightTx); + Preconditions.checkState(success, "Completed transaction %s while %s was submitted", tx, inflightTx); synchronized (this) { - processIfReady(); + final PingPongTransaction nextTx = READY_UPDATER.getAndSet(this, null); + if (nextTx != null) { + processTransaction(nextTx); + } else if (shutdownTx != null) { + processTransaction(shutdownTx); + delegate.close(); + shutdownTx = null; + } } + } + + private void transactionSuccessful(final PingPongTransaction tx, final Void result) { + LOG.debug("Transaction {} completed successfully", tx); - // Can run unsynchronized tx.onSuccess(result); + processNextTransaction(tx); } private void transactionFailed(final PingPongTransaction tx, final Throwable t) { LOG.debug("Transaction {} failed", tx, t); - final boolean success = INFLIGHT_UPDATER.compareAndSet(this, tx, null); - Preconditions.checkState(success, "Failed transaction %s while %s was submitted", tx, inflightTx); - tx.onFailure(t); + processNextTransaction(tx); } private void readyTransaction(@Nonnull final PingPongTransaction tx) { @@ -258,23 +269,29 @@ public final class PingPongTransactionChain implements DOMTransactionChain { final PingPongTransaction notLocked = lockedTx; Preconditions.checkState(notLocked == null, "Attempted to close chain with outstanding transaction %s", notLocked); - // Force allocations on slow path. We will complete the rest - final PingPongTransaction tx = READY_UPDATER.getAndSet(this, null); + // This is not reliable, but if we observe it to be null and the process has already completed, + // the backend transaction chain will throw the appropriate error. + Preconditions.checkState(shutdownTx == null, "Attempted to close an already-closed chain"); - // Make sure no transaction is outstanding. Otherwise sleep a bit and retry - while (inflightTx != null) { - LOG.debug("Busy-waiting for in-flight transaction {} to complete", inflightTx); - Thread.yield(); - continue; - } + // Force allocations on slow path, picking up a potentially-outstanding transaction + final PingPongTransaction tx = READY_UPDATER.getAndSet(this, null); - // If we have an outstanding transaction, send it down if (tx != null) { - processTransaction(tx); + // We have one more transaction, which needs to be processed somewhere. If we do not + // a transaction in-flight, we need to push it down ourselves. + // If there is an in-flight transaction we will schedule this last one into a dedicated + // slot. Allocation slow path will check its presence and fail, the in-flight path will + // pick it up, submit and immediately close the chain. + if (inflightTx == null) { + processTransaction(tx); + delegate.close(); + } else { + shutdownTx = tx; + } + } else { + // Nothing outstanding, we can safely shutdown + delegate.close(); } - - // All done, close the delegate. All new allocations should fail. - delegate.close(); } @Override -- 2.36.6