Fix memory leak in BA mount service

The cache used in BA mount service uses weakKeys() so the keys (DOM mountpoint)
could be GCed when no ono, except cache, referenced them).
However the values in the cache (BA mountpoint) also kept a reference to
the key, so the key was never weakly reachable and thus never GCed
resulting in a memory leak.

Which is especially visible in case of frequent reconnects of netconf
mountpoints where after every reconnect a new DOM mountpoint is created,
requiring a new BA mountpoint creating new entry in the cache of BA mount service...

Added also a simple test verifying proper cache cleanup

Change-Id: I9c109dd6d499d6185842d6a1ad5a59d75565af5c
Signed-off-by: Maros Marsalek <mmarsalek@frinx.io>

diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingDOMMountPointServiceAdapter.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingDOMMountPointServiceAdapter.java
index 244ab51199bbc1ba10e4b6555f839b80a46882f6..628ccb1725987a922c6311d4277a5b4782657e75 100644 (file)
--- a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingDOMMountPointServiceAdapter.java
+++ b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingDOMMountPointServiceAdapter.java
@@ -7,6 +7,7 @@
  */
 package org.opendaylight.controller.md.sal.binding.impl;
 
  */
 package org.opendaylight.controller.md.sal.binding.impl;
 

+import com.google.common.annotations.VisibleForTesting;

 import com.google.common.base.Optional;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.base.Optional;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;


@@ -26,8 +27,9 @@ public class BindingDOMMountPointServiceAdapter implements MountPointService {
 
     private final BindingToNormalizedNodeCodec codec;
     private final DOMMountPointService mountService;
 
     private final BindingToNormalizedNodeCodec codec;
     private final DOMMountPointService mountService;

-    private final LoadingCache<DOMMountPoint, BindingMountPointAdapter> bindingMountpoints = CacheBuilder.newBuilder()
-            .weakKeys().build(new CacheLoader<DOMMountPoint, BindingMountPointAdapter>() {
+    @VisibleForTesting
+    final LoadingCache<DOMMountPoint, BindingMountPointAdapter> bindingMountpoints = CacheBuilder.newBuilder()
+            .weakKeys().weakValues().build(new CacheLoader<DOMMountPoint, BindingMountPointAdapter>() {

                 @Override
                 public BindingMountPointAdapter load(final DOMMountPoint key) {
                     return new BindingMountPointAdapter(codec, key);
                 @Override
                 public BindingMountPointAdapter load(final DOMMountPoint key) {
                     return new BindingMountPointAdapter(codec, key);

diff --git a/opendaylight/md-sal/sal-binding-broker/src/test/java/org/opendaylight/controller/md/sal/binding/impl/BindingDOMMountPointServiceAdapterTest.java b/opendaylight/md-sal/sal-binding-broker/src/test/java/org/opendaylight/controller/md/sal/binding/impl/BindingDOMMountPointServiceAdapterTest.java
new file mode 100644 (file)
index 0000000..a2ece0b
--- /dev/null
+++ b/opendaylight/md-sal/sal-binding-broker/src/test/java/org/opendaylight/controller/md/sal/binding/impl/BindingDOMMountPointServiceAdapterTest.java
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2019 FRINX and others.  All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.controller.md.sal.binding.impl;
+
+import static org.mockito.Matchers.any;
+
+import com.google.common.base.Optional;
+import com.google.common.cache.LoadingCache;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
+import org.opendaylight.controller.md.sal.dom.api.DOMMountPoint;
+import org.opendaylight.controller.md.sal.dom.api.DOMMountPointService;
+import org.opendaylight.mdsal.binding.dom.codec.impl.BindingNormalizedNodeCodecRegistry;
+import org.opendaylight.mdsal.binding.generator.api.ClassLoadingStrategy;
+import org.opendaylight.yangtools.yang.binding.DataObject;
+import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
+import org.opendaylight.yangtools.yang.common.QName;
+import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
+
+public class BindingDOMMountPointServiceAdapterTest {
+
+    @Mock
+    private DOMMountPointService mountService;
+    // Use a real instance of codec, since its getCodecRegistry() method is final and cannot be mocked
+    private BindingToNormalizedNodeCodec codec;
+    @Mock
+    private BindingNormalizedNodeCodecRegistry codecRegistry;
+    @Mock
+    private ClassLoadingStrategy classLoadingStrategy;
+
+    @Before
+    public void setUp() {
+        MockitoAnnotations.initMocks(this);
+        codec = Mockito.spy(new BindingToNormalizedNodeCodec(classLoadingStrategy, codecRegistry));
+        Mockito.doAnswer(invocationOnMock -> Optional.of(Mockito.mock(DOMMountPoint.class)))
+                .when(mountService).getMountPoint(any(YangInstanceIdentifier.class));
+        Mockito.doReturn(YangInstanceIdentifier.create(new YangInstanceIdentifier.NodeIdentifier(QName.create("(a)b"))))
+                .when(codec).toYangInstanceIdentifierBlocking(any(InstanceIdentifier.class));
+        Mockito.doReturn(InstanceIdentifier.create(DataObject.class))
+                .when(codecRegistry).fromYangInstanceIdentifier(any(YangInstanceIdentifier.class));
+    }
+
+    @Test(timeout = 30 * 1000)
+    public void testCaching() throws Exception {
+        BindingDOMMountPointServiceAdapter baService = new BindingDOMMountPointServiceAdapter(mountService, codec);
+        LoadingCache<DOMMountPoint, BindingMountPointAdapter> cache = baService.bindingMountpoints;
+
+        baService.getMountPoint(InstanceIdentifier.create(DataObject.class));
+
+        while (true) {
+            cache.cleanUp();
+            System.gc();
+            Thread.sleep(100);
+            if (cache.asMap().keySet().size() == 0) {
+                // Cache has been cleared, the single cache entry was garbage collected
+                return;
+            }
+        }
+    }
+}
\ No newline at end of file