Bug 7812: NPE when NetconfDeviceSalProvider.close

Clustered connector close logic could be called twice on master.
Add boolean guard field to prevent this.

Add null check to netconf device sal provider close method
to prevent NPE.

Change-Id: Ib84be162826726169fb254a933781bb39dce604a
Signed-off-by: Andrej Mak <andrej.mak@pantheon.tech>
Signed-off-by: matus.kubica <matus.kubica@pantheon.tech>

diff --git a/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyContext.java b/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyContext.java
index 0f8255cdaa3dc5140f38928877d228d0c4752e15..33c678bc7d60937cd1956daffe9d2baed6706f6d 100644 (file)
--- a/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyContext.java
+++ b/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/NetconfTopologyContext.java
@@ -41,6 +41,7 @@ class NetconfTopologyContext implements ClusterSingletonService {
     private RemoteDeviceConnector remoteDeviceConnector;
     private NetconfNodeManager netconfNodeManager;
     private boolean finalClose = false;
+    private boolean closed = false;
     private boolean isMaster;
 
     private ActorRef masterActorRef;
@@ -90,13 +91,7 @@ class NetconfTopologyContext implements ClusterSingletonService {
             // in case that master changes role to slave, new NodeDeviceManager must be created and listener registered
             netconfNodeManager = createNodeDeviceManager();
         }
-        if (masterActorRef != null) {
-            netconfTopologyDeviceSetup.getActorSystem().stop(masterActorRef);
-            masterActorRef = null;
-        }
-        if (remoteDeviceConnector != null) {
-            remoteDeviceConnector.stopRemoteDeviceConnection();
-        }
+        stopDeviceConnectorAndActor();
 
         return Futures.immediateCheckedFuture(null);
     }
@@ -122,15 +117,8 @@ class NetconfTopologyContext implements ClusterSingletonService {
         if (netconfNodeManager != null) {
             netconfNodeManager.close();
         }
+        stopDeviceConnectorAndActor();
 
-        if (remoteDeviceConnector != null) {
-            remoteDeviceConnector.stopRemoteDeviceConnection();
-        }
-
-        if (masterActorRef != null) {
-            netconfTopologyDeviceSetup.getActorSystem().stop(masterActorRef);
-            masterActorRef = null;
-        }
     }
 
     /**
@@ -166,4 +154,19 @@ class NetconfTopologyContext implements ClusterSingletonService {
             }, netconfTopologyDeviceSetup.getActorSystem().dispatcher());
         }
     }
+
+    private synchronized void stopDeviceConnectorAndActor() {
+        if (closed) {
+            return;
+        }
+        if (remoteDeviceConnector != null) {
+            remoteDeviceConnector.stopRemoteDeviceConnection();
+        }
+
+        if (masterActorRef != null) {
+            netconfTopologyDeviceSetup.getActorSystem().stop(masterActorRef);
+            masterActorRef = null;
+        }
+        closed = true;
+    }
 }

diff --git a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/NetconfDeviceSalProvider.java b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/NetconfDeviceSalProvider.java
index 278771a5ba880a97ec4e3c0eaaa2da7bdb841be4..99e34d5e813748a656d73e353e45bce8e303b2d6 100644 (file)
--- a/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/NetconfDeviceSalProvider.java
+++ b/netconf/sal-netconf-connector/src/main/java/org/opendaylight/netconf/sal/connect/netconf/sal/NetconfDeviceSalProvider.java
@@ -112,9 +112,13 @@ public class NetconfDeviceSalProvider implements AutoCloseable, Provider, Bindin
 
     public void close() throws Exception {
         mountInstance.close();
-        topologyDatastoreAdapter.close();
+        if (topologyDatastoreAdapter != null) {
+            topologyDatastoreAdapter.close();
+        }
         topologyDatastoreAdapter = null;
-        txChain.close();
+        if (txChain != null) {
+            txChain.close();
+        }
     }
 
     public static final class MountInstance implements AutoCloseable {

diff --git a/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/NetconfDeviceSalProviderTest.java b/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/NetconfDeviceSalProviderTest.java
new file mode 100644 (file)
index 0000000..b49b3fd
--- /dev/null
+++ b/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/sal/NetconfDeviceSalProviderTest.java
@@ -0,0 +1,105 @@
+/*
+ * Copyright (c) 2016 Cisco Systems, Inc. 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.netconf.sal.connect.netconf.sal;
+
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import com.google.common.util.concurrent.Futures;
+import java.net.InetSocketAddress;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.opendaylight.controller.md.sal.binding.api.BindingTransactionChain;
+import org.opendaylight.controller.md.sal.binding.api.DataBroker;
+import org.opendaylight.controller.md.sal.binding.api.WriteTransaction;
+import org.opendaylight.controller.md.sal.common.api.data.TransactionChainListener;
+import org.opendaylight.controller.md.sal.dom.api.DOMMountPointService;
+import org.opendaylight.controller.sal.binding.api.BindingAwareBroker;
+import org.opendaylight.controller.sal.core.api.Broker;
+import org.opendaylight.netconf.sal.connect.util.RemoteDeviceId;
+
+public class NetconfDeviceSalProviderTest {
+
+    @Mock
+    private Broker.ProviderSession session;
+    @Mock
+    private DOMMountPointService mountpointService;
+    @Mock
+    private BindingAwareBroker.ProviderContext context;
+    @Mock
+    private WriteTransaction tx;
+    @Mock
+    private DataBroker dataBroker;
+    @Mock
+    private BindingTransactionChain chain;
+    private NetconfDeviceSalProvider provider;
+
+    @Before
+    public void setUp() throws Exception {
+        MockitoAnnotations.initMocks(this);
+        provider = new NetconfDeviceSalProvider(new RemoteDeviceId("device1", InetSocketAddress.createUnresolved("localhost", 17830)));
+        when(session.getService(DOMMountPointService.class)).thenReturn(mountpointService);
+        when(context.getSALService(DataBroker.class)).thenReturn(dataBroker);
+        when(dataBroker.createTransactionChain(any())).thenReturn(chain);
+        when(chain.newWriteOnlyTransaction()).thenReturn(tx);
+        when(tx.submit()).thenReturn(Futures.immediateCheckedFuture(null));
+        when(tx.getIdentifier()).thenReturn(tx);
+    }
+
+    @Test
+    public void onSessionInitiated() throws Exception {
+        provider.onSessionInitiated(session);
+        provider.onSessionInitiated(context);
+        Assert.assertNotNull(provider.getMountInstance());
+        Assert.assertNotNull(provider.getTopologyDatastoreAdapter());
+    }
+
+    @Test
+    public void getProviderFunctionality() throws Exception {
+        Assert.assertTrue(provider.getProviderFunctionality().isEmpty());
+    }
+
+    @Test
+    public void replaceChainIfFailed() throws Exception {
+        provider.onSessionInitiated(session);
+        provider.onSessionInitiated(context);
+        Assert.assertNotNull(provider.getMountInstance());
+        final ArgumentCaptor<TransactionChainListener> captor = ArgumentCaptor.forClass(TransactionChainListener.class);
+        verify(dataBroker).createTransactionChain(captor.capture());
+        try {
+            captor.getValue().onTransactionChainFailed(chain, tx, new Exception("chain failed"));
+        } catch (final IllegalStateException e) {
+            //expected
+        }
+        verify(dataBroker, times(2)).createTransactionChain(any());
+    }
+
+    @Test
+    public void close() throws Exception {
+        provider.onSessionInitiated(session);
+        provider.onSessionInitiated(context);
+        provider.close();
+        verify(chain).close();
+    }
+
+    @Test
+    public void closeWithoutNPE() throws Exception {
+        provider.onSessionInitiated(session);
+        provider.onSessionInitiated(context);
+        provider.close();
+        provider.close();
+        verify(chain, times(2)).close();
+    }
+}
\ No newline at end of file