From: Tom Pantelis <tompantelis@gmail.com>
Date: Tue, 21 Nov 2017 22:50:27 +0000 (-0500)
Subject: NETVIRT-937: Fix NPE in ElanInstanceManager
X-Git-Tag: release/oxygen~197
X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=refs%2Fchanges%2F21%2F65821%2F2;p=netvirt.git

NETVIRT-937: Fix NPE in ElanInstanceManager

2017-11-21 16:21:43,179 | ERROR | nPool-1-worker-2 | JobCoordinatorImpl               | 261 - org.opendaylight.infrautils.jobcoordinator-impl - 1.3.0.SNAPSHOT | Runnnable likely about to terminate thread due to uncaught exception; but here is useful debugging context: JobEntry{key='elaninterface-963a4d35-297e-4757-be14-6c3dec354221', mainWorker=org.opendaylight.netvirt.elan.internal.ElanInstanceManagerElanInstanceManager$$Lambda$774/1098386956@516cbba2, rollbackWorker=null, retryCount=6, futures=[null, com.google.common.util.concurrent.ImmediateFuture$ImmediateSuccessfulCheckedFuture@39325841]}
java.lang.NullPointerException: at index 0
	at com.google.common.collect.ObjectArrays.checkElementNotNull(ObjectArrays.java:235)[27:com.google.guava:22.0.0]
	at com.google.common.collect.ObjectArrays.checkElementsNotNull(ObjectArrays.java:225)[27:com.google.guava:22.0.0]
	at com.google.common.collect.ObjectArrays.checkElementsNotNull(ObjectArrays.java:219)[27:com.google.guava:22.0.0]
	at com.google.common.collect.ImmutableList.construct(ImmutableList.java:342)[27:com.google.guava:22.0.0]
	at com.google.common.collect.ImmutableList.copyOf(ImmutableList.java:257)[27:com.google.guava:22.0.0]
	at com.google.common.collect.ImmutableList.copyOf(ImmutableList.java:228)[27:com.google.guava:22.0.0]
	at com.google.common.util.concurrent.Futures.allAsList(Futures.java:835)[27:com.google.guava:22.0.0]
	at org.opendaylight.infrautils.jobcoordinator.internal.JobCoordinatorImpl$MainTask.runWithUncheckedExceptionLogging(JobCoordinatorImpl.java:369)[261:org.opendaylight.infrautils.jobcoordinator-impl:1.3.0.SNAPSHOT]

The futures List in remove is reused across the submitted jobs in the forEach loop
which is unsafe as the List is accessed by multiple threads. Each job now returns its
own local futures List.

The futures returned from elanInterfaceManager.removeElanInterface
were also added to the List but I don't see the purpose of this. removeElanInterface
waits for those tx's to complete. Maybe it was so the JC would retry them? If so, this
entire code path has a mix of sync and async transactions which is a bit confusing -
some are submitted to the JC, ohers aren't; some may be retried, others aren't.

Change-Id: I82193fc36a339db42b83df5bfa3d17ff0a88dfa0
Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
---

diff --git a/vpnservice/elanmanager/elanmanager-impl/src/main/java/org/opendaylight/netvirt/elan/internal/ElanInstanceManager.java b/vpnservice/elanmanager/elanmanager-impl/src/main/java/org/opendaylight/netvirt/elan/internal/ElanInstanceManager.java
index 61178379b9..081a636b59 100644
--- a/vpnservice/elanmanager/elanmanager-impl/src/main/java/org/opendaylight/netvirt/elan/internal/ElanInstanceManager.java
+++ b/vpnservice/elanmanager/elanmanager-impl/src/main/java/org/opendaylight/netvirt/elan/internal/ElanInstanceManager.java
@@ -11,11 +11,9 @@ package org.opendaylight.netvirt.elan.internal;
 import static java.util.Collections.emptyList;
 
 import com.google.common.base.Optional;
-import com.google.common.util.concurrent.ListenableFuture;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-
 import javax.annotation.Nonnull;
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
@@ -77,7 +75,6 @@ public class ElanInstanceManager extends AsyncDataTreeChangeListenerBase<ElanIns
     @Override
     protected void remove(InstanceIdentifier<ElanInstance> identifier, ElanInstance deletedElan) {
         LOG.trace("Remove ElanInstance - Key: {}, value: {}", identifier, deletedElan);
-        List<ListenableFuture<Void>> futures = new ArrayList<>();
         String elanName = deletedElan.getElanInstanceName();
         // check the elan Instance present in the Operational DataStore
         Elan existingElan = ElanUtils.getElanByName(broker, elanName);
@@ -90,8 +87,7 @@ public class ElanInstanceManager extends AsyncDataTreeChangeListenerBase<ElanIns
                     InstanceIdentifier<ElanInterface> elanInterfaceId = ElanUtils
                             .getElanInterfaceConfigurationDataPathId(elanInterfaceName);
                     InterfaceInfo interfaceInfo = interfaceManager.getInterfaceInfo(elanInterfaceName);
-                    futures.addAll(elanInterfaceManager.removeElanInterface(deletedElan, elanInterfaceName,
-                            interfaceInfo, false));
+                    elanInterfaceManager.removeElanInterface(deletedElan, elanInterfaceName, interfaceInfo, false);
                     ElanUtils.delete(broker, LogicalDatastoreType.CONFIGURATION,
                             elanInterfaceId);
                 }
@@ -118,8 +114,7 @@ public class ElanInstanceManager extends AsyncDataTreeChangeListenerBase<ElanIns
                 elanInterfaceManager.unbindService(elanInterfaceName, writeConfigTxn);
                 ElanUtils.removeElanInterfaceToElanInstanceCache(elanName, elanInterfaceName);
                 LOG.info("unbind the Interface:{} service bounded to Elan:{}", elanInterfaceName, elanName);
-                futures.add(writeConfigTxn.submit());
-                return futures;
+                return Collections.singletonList(writeConfigTxn.submit());
             }, ElanConstants.JOB_MAX_RETRIES);
         });
         // Release tag