Use java.lang.ref.Cleaner for ClientBackedTransaction

Improve temporary file clean up by using a Cleaner to dispatch
cleaning tasks. Since it gives us a Cleanable, we can dispense with
a tracking map and removal from it -- Cleanable makes sure it is
called exactly once.

JIRA: CONTROLLER-1911
Change-Id: I02d2ee57a9fada2c54ba06008b6c28681a709748
Signed-off-by: tadei.bilan <tadei.bilan@pantheon.tech>
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/ClientBackedTransaction.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/ClientBackedTransaction.java
index 9b65a881ec8d2d9074919086a80bb117c8aa8c28..a0f369638d50e17aaa7882fec9137bab9c48cae9 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/ClientBackedTransaction.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/ClientBackedTransaction.java
@@ -9,16 +9,13 @@ package org.opendaylight.controller.cluster.databroker;
 
 import static java.util.Objects.requireNonNull;
 
-import com.google.common.base.FinalizablePhantomReference;
-import com.google.common.base.FinalizableReferenceQueue;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import org.eclipse.jdt.annotation.NonNull;
-import org.eclipse.jdt.annotation.Nullable;
+import java.lang.ref.Cleaner;
+import java.lang.ref.Cleaner.Cleanable;
 import org.opendaylight.controller.cluster.access.concepts.TransactionIdentifier;
 import org.opendaylight.controller.cluster.databroker.actors.dds.AbstractClientHandle;
 import org.opendaylight.controller.cluster.databroker.actors.dds.ClientTransaction;
 import org.opendaylight.mdsal.dom.spi.store.AbstractDOMStoreTransaction;
+import org.opendaylight.mdsal.dom.spi.store.DOMStoreTransaction;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -31,50 +28,43 @@ import org.slf4j.LoggerFactory;
  */
 abstract class ClientBackedTransaction<T extends AbstractClientHandle<?>> extends
         AbstractDOMStoreTransaction<TransactionIdentifier> {
-    private static final class Finalizer extends FinalizablePhantomReference<ClientBackedTransaction<?>> {
-        private static final FinalizableReferenceQueue QUEUE = new FinalizableReferenceQueue();
-        private static final Set<Finalizer> FINALIZERS = ConcurrentHashMap.newKeySet();
-        private static final Logger LOG = LoggerFactory.getLogger(Finalizer.class);
-
+    private static final class Cleanup implements Runnable {
         private final AbstractClientHandle<?> transaction;
         private final Throwable allocationContext;
 
-        private Finalizer(final ClientBackedTransaction<?> referent, final AbstractClientHandle<?> transaction,
-                final Throwable allocationContext) {
-            super(referent, QUEUE);
-            this.transaction = requireNonNull(transaction);
+        Cleanup(final AbstractClientHandle<?> transaction, final Throwable allocationContext) {
+            this.transaction = transaction;
             this.allocationContext = allocationContext;
         }
 
-        static <T extends AbstractClientHandle<?>> @NonNull T recordTransaction(
-                final @NonNull ClientBackedTransaction<T> referent, final @NonNull T transaction,
-                final @Nullable Throwable allocationContext) {
-            FINALIZERS.add(new Finalizer(referent, transaction, allocationContext));
-            return transaction;
-        }
-
         @Override
-        public void finalizeReferent() {
-            FINALIZERS.remove(this);
+        public void run() {
             if (transaction.abort()) {
                 LOG.info("Aborted orphan transaction {}", transaction, allocationContext);
             }
         }
     }
 
+    private static final Logger LOG = LoggerFactory.getLogger(ClientBackedTransaction.class);
+    private static final Cleaner CLEANER = Cleaner.create();
+
     private final T delegate;
+    private final Cleanable cleanable;
 
     ClientBackedTransaction(final T delegate, final Throwable allocationContext) {
         super(delegate.getIdentifier());
-        this.delegate = Finalizer.recordTransaction(this, delegate, allocationContext);
-    }
-
-    final T delegate() {
-        return delegate;
+        this.delegate = requireNonNull(delegate);
+        this.cleanable = CLEANER.register(this, new Cleanup(delegate, allocationContext));
     }
 
     @Override
     public void close() {
         delegate.abort();
+        // Run cleaning immediate so the references is not stuck in cleaner queue
+        cleanable.clean();
+    }
+
+    final T delegate() {
+        return delegate;
     }
 }

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/AbstractClientHandle.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/AbstractClientHandle.java
index 90773359dcab9c47089f6bdd96c1675c4275d5b7..842bc00d916ab1eae6c531cb4ceeddc971bdbc4c 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/AbstractClientHandle.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/AbstractClientHandle.java
@@ -97,9 +97,10 @@ public abstract class AbstractClientHandle<T extends AbstractProxyTransaction> e
      *         closed, too.
      */
     final @Nullable Collection<T> ensureClosed() {
-        @SuppressWarnings("unchecked")
-        final State<T> local = STATE_UPDATER.getAndSet(this, null);
-        return local == null ? null : local.values();
+        // volatile read and a conditional CAS. This ends up being better in the typical case when we are invoked more
+        // than once (see ClientBackedTransaction) than performing a STATE_UPDATER.getAndSet().
+        final State<T> local = state;
+        return local != null && STATE_UPDATER.compareAndSet(this, local, null) ? local.values() : null;
     }
 
     final T ensureProxy(final YangInstanceIdentifier path) {

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/ClientBackedTransactionTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/ClientBackedTransactionTest.java
index 39e8d74dcee8f8d42f8576891d94dde019d40afe..be140eb625dfe02ec681c851bcb1ba2f420a60ba 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/ClientBackedTransactionTest.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/ClientBackedTransactionTest.java
@@ -7,8 +7,10 @@
  */
 package org.opendaylight.controller.cluster.databroker;
 
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
 import org.junit.Test;
-import org.mockito.Mockito;
 import org.opendaylight.controller.cluster.access.concepts.ClientIdentifier;
 import org.opendaylight.controller.cluster.access.concepts.FrontendIdentifier;
 import org.opendaylight.controller.cluster.access.concepts.FrontendType;
@@ -31,6 +33,7 @@ public abstract class ClientBackedTransactionTest<T extends ClientBackedTransact
     public void testClose() {
         final AbstractClientHandle<?> delegate = object().delegate();
         object().close();
-        Mockito.verify(delegate).abort();
+        // Called twice because of immediate cleaning
+        verify(delegate, times(2)).abort();
     }
 }
\ No newline at end of file