From 7eead013cff29f7a15d9be546900548363f492ee Mon Sep 17 00:00:00 2001 From: Tony Tkacik Date: Thu, 4 Sep 2014 16:18:46 +0200 Subject: [PATCH] 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 --- .../BindingTranslatedTransactionChain.java | 96 ++++++++++++------- 1 file changed, 59 insertions(+), 37 deletions(-) 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 2d8e51cff9..d16170ba48 100644 --- 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 { private final DOMTransactionChain delegate; - - @GuardedBy("this") - private final Map, 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 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(delegateTx, codec) { + + @Override + public CheckedFuture submit() { + return listenForFailure(this,super.submit()); + }; + + }; return bindingTx; } - @Override - public void close() { - delegate.close(); + protected CheckedFuture listenForFailure( + final WriteTransaction tx, CheckedFuture future) { + Futures.addCallback(future, new FutureCallback() { + @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 -- 2.36.6