From b1e3abf27292895d8921398ebb9cf66072681f83 Mon Sep 17 00:00:00 2001
From: adetalhouet <adetalhouet@inocybe.com>
Date: Thu, 17 Dec 2015 11:31:07 -0500
Subject: [PATCH] Bug 4709 - Add keepalive response timeout to
 KeepaliveSalFacade of netconf connector.

Change-Id: Ie6c7593eebd62c85430b2b219921fa7d28e63024
Signed-off-by: adetalhouet <adetalhouet@inocybe.com>
---
 .../netconf/sal-netconf-connector/pom.xml     | 14 +++---
 .../netconf/sal/KeepaliveSalFacade.java       |  7 ++-
 .../netconf/sal/KeepaliveSalFacadeTest.java   | 46 +++++++++++++++----
 3 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/opendaylight/netconf/sal-netconf-connector/pom.xml b/opendaylight/netconf/sal-netconf-connector/pom.xml
index 418cfc3de6..d53eee48b8 100644
--- a/opendaylight/netconf/sal-netconf-connector/pom.xml
+++ b/opendaylight/netconf/sal-netconf-connector/pom.xml
@@ -164,15 +164,15 @@
       <scope>test</scope>
     </dependency>
     <dependency>
-      <groupId>org.opendaylight.controller</groupId>
-      <artifactId>sal-binding-broker-impl</artifactId>
-      <type>test-jar</type>
-      <scope>test</scope>
+        <groupId>org.opendaylight.controller</groupId>
+        <artifactId>sal-binding-broker-impl</artifactId>
+        <type>test-jar</type>
+        <scope>test</scope>
     </dependency>
     <dependency>
-      <groupId>org.opendaylight.yangtools</groupId>
-      <artifactId>mockito-configuration</artifactId>
-      <scope>test</scope>
+        <groupId>org.mockito</groupId>
+        <artifactId>mockito-core</artifactId>
+        <scope>test</scope>
     </dependency>
   </dependencies>
 
diff --git a/opendaylight/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacade.java b/opendaylight/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacade.java
index 862b860eef..3677ba41e9 100644
--- a/opendaylight/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacade.java
+++ b/opendaylight/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacade.java
@@ -173,7 +173,12 @@ public final class KeepaliveSalFacade implements RemoteDeviceHandler<NetconfSess
             LOG.trace("{}: Invoking keepalive RPC", id);
 
             try {
-                Futures.addCallback(currentDeviceRpc.invokeRpc(PATH, KEEPALIVE_PAYLOAD), this);
+                if(!currentKeepalive.isDone()) {
+                    onFailure(new IllegalStateException("Previous keepalive timed out"));
+                } else {
+                    Futures.addCallback(currentDeviceRpc.invokeRpc(PATH, KEEPALIVE_PAYLOAD), this);
+                    scheduleKeepalive();
+                }
             } catch (NullPointerException e) {
                 LOG.debug("{}: Skipping keepalive while reconnecting", id);
                 // Empty catch block intentional
diff --git a/opendaylight/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacadeTest.java b/opendaylight/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacadeTest.java
index 5dda13a956..1bb2f7a732 100644
--- a/opendaylight/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacadeTest.java
+++ b/opendaylight/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/KeepaliveSalFacadeTest.java
@@ -16,13 +16,18 @@ import static org.mockito.Mockito.timeout;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
-import com.google.common.util.concurrent.Futures;
 import java.net.InetSocketAddress;
 import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.Matchers;
 import org.mockito.Mock;
+import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
@@ -41,6 +46,8 @@ import org.opendaylight.yangtools.yang.data.impl.schema.Builders;
 import org.opendaylight.yangtools.yang.model.api.SchemaContext;
 import org.opendaylight.yangtools.yang.model.api.SchemaPath;
 
+import com.google.common.util.concurrent.Futures;
+
 public class KeepaliveSalFacadeTest {
 
     private static final RemoteDeviceId REMOTE_DEVICE_ID = new RemoteDeviceId("test", new InetSocketAddress("localhost", 22));
@@ -48,7 +55,7 @@ public class KeepaliveSalFacadeTest {
     @Mock
     private RemoteDeviceHandler<NetconfSessionPreferences> underlyingSalFacade;
 
-    private static java.util.concurrent.ScheduledExecutorService executorService;
+    private ScheduledExecutorService executorServiceSpy;
 
     @Mock
     private NetconfDeviceCommunicator listener;
@@ -57,9 +64,12 @@ public class KeepaliveSalFacadeTest {
 
     private DOMRpcService proxyRpc;
 
+    @Mock
+    private ScheduledFuture currentKeepalive;
+
     @Before
     public void setUp() throws Exception {
-        executorService = Executors.newScheduledThreadPool(1);
+        executorServiceSpy = Executors.newScheduledThreadPool(1);
 
         MockitoAnnotations.initMocks(this);
 
@@ -67,11 +77,25 @@ public class KeepaliveSalFacadeTest {
         doReturn("mockedRpc").when(deviceRpc).toString();
         doNothing().when(underlyingSalFacade).onDeviceConnected(
                 any(SchemaContext.class), any(NetconfSessionPreferences.class), any(DOMRpcService.class));
+
+        ScheduledExecutorService executorService = Executors.newScheduledThreadPool(1);
+        executorServiceSpy = Mockito.spy(executorService);
+        doAnswer(new Answer<ScheduledFuture>() {
+            @Override
+            public ScheduledFuture answer(InvocationOnMock invocationOnMock)
+                    throws Throwable {
+                invocationOnMock.callRealMethod();
+                return currentKeepalive;
+            }
+        }).when(executorServiceSpy).schedule(Mockito.<Runnable> any(),
+                Mockito.anyLong(), Matchers.<TimeUnit> any());
+
+        Mockito.when(currentKeepalive.isDone()).thenReturn(true);
     }
 
     @After
     public void tearDown() throws Exception {
-        executorService.shutdownNow();
+        executorServiceSpy.shutdownNow();
     }
 
     @Test
@@ -82,7 +106,7 @@ public class KeepaliveSalFacadeTest {
         doReturn(Futures.immediateCheckedFuture(result)).when(deviceRpc).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
 
         final KeepaliveSalFacade keepaliveSalFacade =
-                new KeepaliveSalFacade(REMOTE_DEVICE_ID, underlyingSalFacade, executorService, 1L);
+                new KeepaliveSalFacade(REMOTE_DEVICE_ID, underlyingSalFacade, executorServiceSpy, 1L);
         keepaliveSalFacade.setListener(listener);
 
         keepaliveSalFacade.onDeviceConnected(null, null, deviceRpc);
@@ -90,7 +114,7 @@ public class KeepaliveSalFacadeTest {
         verify(underlyingSalFacade).onDeviceConnected(
                 any(SchemaContext.class), any(NetconfSessionPreferences.class), any(DOMRpcService.class));
 
-        verify(deviceRpc, timeout(15000).times(5)).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
+        verify(deviceRpc, timeout(15000).atLeast(5)).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
     }
 
     @Test
@@ -109,7 +133,7 @@ public class KeepaliveSalFacadeTest {
                 .when(deviceRpc).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
 
         final KeepaliveSalFacade keepaliveSalFacade =
-                new KeepaliveSalFacade(REMOTE_DEVICE_ID, underlyingSalFacade, executorService, 1L);
+                new KeepaliveSalFacade(REMOTE_DEVICE_ID, underlyingSalFacade, executorServiceSpy, 1L);
         keepaliveSalFacade.setListener(listener);
 
         keepaliveSalFacade.onDeviceConnected(null, null, deviceRpc);
@@ -144,6 +168,12 @@ public class KeepaliveSalFacadeTest {
 
         // 1 failed that results in disconnect, 3 total with previous fail
         verify(listener, timeout(15000).times(3)).disconnect();
+
+
+        Mockito.when(currentKeepalive.isDone()).thenReturn(false);
+        keepaliveSalFacade.onDeviceConnected(null, null, deviceRpc);
+        // 1 failed that results in disconnect, 4 total with previous fail
+        verify(listener, timeout(15000).times(4)).disconnect();
     }
 
     @Test
@@ -160,7 +190,7 @@ public class KeepaliveSalFacadeTest {
                 .when(deviceRpc).invokeRpc(any(SchemaPath.class), any(NormalizedNode.class));
 
         final KeepaliveSalFacade keepaliveSalFacade =
-                new KeepaliveSalFacade(REMOTE_DEVICE_ID, underlyingSalFacade, executorService, 100L);
+                new KeepaliveSalFacade(REMOTE_DEVICE_ID, underlyingSalFacade, executorServiceSpy, 100L);
         keepaliveSalFacade.setListener(listener);
 
         keepaliveSalFacade.onDeviceConnected(null, null, deviceRpc);
-- 
2.36.6