Bug 1703: Fixed memory leak in TransactionChain

As it turned out WeakHashMap is not cleared
if value has strong reference to key. This
required to rewrite BindingTransactionChain
to listen on submit futures for each  transaction
in order to receive events mapped to binding transaction.

This actually simplifies code and removes
any need for having canonical mapping from
DOM transaction to Binding transaction.

Change-Id: I9aa4125197b022dd163f85c6965ad1227e771b99
Signed-off-by: Tony Tkacik <ttkacik@cisco.com>

diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingTranslatedTransactionChain.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingTranslatedTransactionChain.java
index 2d8e51cff9d1a1c00e50a03aa671a9d97fce133e..d16170ba481d7d1a295a6acdcf33ae83b73b8a7d 100644 (file)
--- a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingTranslatedTransactionChain.java
+++ b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingTranslatedTransactionChain.java
@@ -7,11 +7,10 @@
  */
 package org.opendaylight.controller.md.sal.binding.impl;
 
-import java.util.Map;
-import java.util.WeakHashMap;
-
-import javax.annotation.concurrent.GuardedBy;
-
+import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.CheckedFuture;
+import com.google.common.util.concurrent.FutureCallback;
+import com.google.common.util.concurrent.Futures;
 import org.opendaylight.controller.md.sal.binding.api.BindingTransactionChain;
 import org.opendaylight.controller.md.sal.binding.api.ReadOnlyTransaction;
 import org.opendaylight.controller.md.sal.binding.api.ReadWriteTransaction;
@@ -19,6 +18,7 @@ import org.opendaylight.controller.md.sal.binding.api.WriteTransaction;
 import org.opendaylight.controller.md.sal.common.api.data.AsyncTransaction;
 import org.opendaylight.controller.md.sal.common.api.data.TransactionChain;
 import org.opendaylight.controller.md.sal.common.api.data.TransactionChainListener;
+import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException;
 import org.opendaylight.controller.md.sal.dom.api.DOMDataBroker;
 import org.opendaylight.controller.md.sal.dom.api.DOMDataReadOnlyTransaction;
 import org.opendaylight.controller.md.sal.dom.api.DOMDataReadWriteTransaction;
@@ -26,20 +26,19 @@ import org.opendaylight.controller.md.sal.dom.api.DOMDataWriteTransaction;
 import org.opendaylight.controller.md.sal.dom.api.DOMTransactionChain;
 import org.opendaylight.yangtools.concepts.Delegator;
 
-import com.google.common.base.Preconditions;
-
 class BindingTranslatedTransactionChain implements BindingTransactionChain, Delegator<DOMTransactionChain> {
 
     private final DOMTransactionChain delegate;
-
-    @GuardedBy("this")
-    private final Map<AsyncTransaction<?, ?>, AsyncTransaction<?, ?>> delegateTxToBindingTx = new WeakHashMap<>();
     private final BindingToNormalizedNodeCodec codec;
+    private final DelegateChainListener delegatingListener;
+    private final TransactionChainListener listener;
 
     public BindingTranslatedTransactionChain(final DOMDataBroker chainFactory,
             final BindingToNormalizedNodeCodec codec, final TransactionChainListener listener) {
         Preconditions.checkNotNull(chainFactory, "DOM Transaction chain factory must not be null");
-        this.delegate = chainFactory.createTransactionChain(new ListenerInvoker(listener));
+        this.delegatingListener = new DelegateChainListener();
+        this.listener = listener;
+        this.delegate = chainFactory.createTransactionChain(listener);
         this.codec = codec;
     }
 
@@ -52,56 +51,79 @@ class BindingTranslatedTransactionChain implements BindingTransactionChain, Dele
     public ReadOnlyTransaction newReadOnlyTransaction() {
         DOMDataReadOnlyTransaction delegateTx = delegate.newReadOnlyTransaction();
         ReadOnlyTransaction bindingTx = new BindingDataReadTransactionImpl(delegateTx, codec);
-        putDelegateToBinding(delegateTx, bindingTx);
         return bindingTx;
     }
 
     @Override
     public ReadWriteTransaction newReadWriteTransaction() {
         DOMDataReadWriteTransaction delegateTx = delegate.newReadWriteTransaction();
-        ReadWriteTransaction bindingTx = new BindingDataReadWriteTransactionImpl(delegateTx, codec);
-        putDelegateToBinding(delegateTx, bindingTx);
+        ReadWriteTransaction bindingTx = new BindingDataReadWriteTransactionImpl(delegateTx, codec) {
+
+            @Override
+            public CheckedFuture<Void, TransactionCommitFailedException> submit() {
+                return listenForFailure(this,super.submit());
+            }
+
+        };
         return bindingTx;
     }
 
     @Override
     public WriteTransaction newWriteOnlyTransaction() {
-        DOMDataWriteTransaction delegateTx = delegate.newWriteOnlyTransaction();
-        WriteTransaction bindingTx = new BindingDataWriteTransactionImpl<>(delegateTx, codec);
-        putDelegateToBinding(delegateTx, bindingTx);
+        final DOMDataWriteTransaction delegateTx = delegate.newWriteOnlyTransaction();
+        WriteTransaction bindingTx = new BindingDataWriteTransactionImpl<DOMDataWriteTransaction>(delegateTx, codec) {
+
+            @Override
+            public CheckedFuture<Void,TransactionCommitFailedException> submit() {
+                return listenForFailure(this,super.submit());
+            };
+
+        };
         return bindingTx;
     }
 
-    @Override
-    public void close() {
-        delegate.close();
+    protected CheckedFuture<Void, TransactionCommitFailedException> listenForFailure(
+            final WriteTransaction tx, CheckedFuture<Void, TransactionCommitFailedException> future) {
+        Futures.addCallback(future, new FutureCallback<Void>() {
+            @Override
+            public void onFailure(Throwable t) {
+                failTransactionChain(tx,t);
+            }
+
+            @Override
+            public void onSuccess(Void result) {
+                // Intentionally NOOP
+            }
+        });
+
+        return future;
     }
 
-    private synchronized void putDelegateToBinding(final AsyncTransaction<?, ?> domTx,
-            final AsyncTransaction<?, ?> bindingTx) {
-        final Object previous = delegateTxToBindingTx.put(domTx, bindingTx);
-        Preconditions.checkState(previous == null, "DOM Transaction %s has already associated binding transation %s",domTx,previous);
+    protected void failTransactionChain(WriteTransaction tx, Throwable t) {
+        // We asume correct state change for underlaying transaction
+        // chain, so we are not changing any of our internal state
+        // to mark that we failed.
+        this.delegatingListener.onTransactionChainFailed(this, tx, t);
     }
 
-    private synchronized AsyncTransaction<?, ?> getBindingTransaction(final AsyncTransaction<?, ?> transaction) {
-        return delegateTxToBindingTx.get(transaction);
+    @Override
+    public void close() {
+        delegate.close();
     }
 
-    private final class ListenerInvoker implements TransactionChainListener {
-
-        private final TransactionChainListener listener;
-
-        public ListenerInvoker(final TransactionChainListener listener) {
-            this.listener = Preconditions.checkNotNull(listener, "Listener must not be null.");
-        }
+    private final class DelegateChainListener implements TransactionChainListener {
 
         @Override
         public void onTransactionChainFailed(final TransactionChain<?, ?> chain,
                 final AsyncTransaction<?, ?> transaction, final Throwable cause) {
-            Preconditions.checkState(delegate.equals(chain),
-                    "Illegal state - listener for %s was invoked for incorrect chain %s.", delegate, chain);
-            AsyncTransaction<?, ?> bindingTx = getBindingTransaction(transaction);
-            listener.onTransactionChainFailed(chain, bindingTx, cause);
+            /*
+             * Intentionally NOOP, callback for failure, since we
+             * are also listening on each transaction for failure.
+             *
+             * by listening on submit future for Binding transaction
+             * in order to provide Binding transaction (which was seen by client
+             * of this transaction chain, instead of
+            */
         }
 
         @Override