commit 6256d01c1cce111998c2962d563e8d3c1bd4a235
author Carmelo Cascone <carmelo@onlab.us> Fri Jun 17 13:49:52 2016 -0700
committer Jonathan Hart <jono@onlab.us> Tue Jun 28 23:04:51 2016 +0000
tree 41f23fe8d64037fe96b20ef499a084ce1283323e
parent 785fada2fb5fd481323f6a48e47aa8c0b7328e54

Fixed deadlock in BMv2 device context service

Also minor refactoring of synchronized blocks.

Change-Id: Ifea25208ca4f1839bb3f21ba5b5ecfb2441baa35

protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java

diff --git a/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java b/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java
index 4727913..c6f3d04 100644
--- a/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java
+++ b/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2DeviceContextServiceImpl.java
@@ -60,6 +60,8 @@
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 import static org.onosproject.bmv2.api.context.Bmv2DefaultConfiguration.parse;
@@ -87,7 +89,7 @@
 
     private final ScheduledExecutorService scheduledExecutor = SharedScheduledExecutors.getPoolThreadExecutor();
     private final MapEventListener<DeviceId, Bmv2DeviceContext> contextListener = new ContextMapEventListener();
-    private final ConcurrentMap<DeviceId, Boolean> deviceLocks = Maps.newConcurrentMap();
+    private final ConcurrentMap<DeviceId, Lock> deviceLocks = Maps.newConcurrentMap();
 
     private ConsistentMap<DeviceId, Bmv2DeviceContext> contexts;
     private Map<String, ClassLoader> interpreterClassLoaders;
@@ -115,7 +117,7 @@
         interpreterClassLoaders = Maps.newConcurrentMap();
         registerInterpreterClassLoader(defaultInterpreter.getClass(), this.getClass().getClassLoader());
 
-        contexts.addListener(contextListener);
+        contexts.addListener(contextListener, scheduledExecutor);
 
         if (configChecker != null &&  configChecker.isCancelled()) {
             configChecker.cancel(false);
@@ -169,16 +171,14 @@
         return defaultContext;
     }
 
-    private void configCheck(DeviceId deviceId) {
+    private void configCheck(DeviceId deviceId, Bmv2DeviceContext storedContext) {
+        if (storedContext == null) {
+            return;
+        }
         // Synchronize executions over the same deviceId.
-        deviceLocks.putIfAbsent(deviceId, new Boolean(true));
-        synchronized (deviceLocks.get(deviceId)) {
-
-            Bmv2DeviceContext storedContext = getContext(deviceId);
-            if (storedContext == null) {
-                return;
-            }
-
+        Lock lock = deviceLocks.computeIfAbsent(deviceId, did -> new ReentrantLock());
+        lock.lock();
+        try {
             log.trace("Executing configuration check on {}...", deviceId);
 
             try {
@@ -200,18 +200,20 @@
             } catch (Bmv2RuntimeException e) {
                 log.warn("Unable to dump JSON configuration from {}: {}", deviceId, e.explain());
             }
+        } finally {
+            lock.unlock();
         }
     }
 
-    private void triggerConfigCheck(DeviceId deviceId) {
+    private void triggerConfigCheck(DeviceId deviceId, Bmv2DeviceContext context) {
         if (mastershipService.isLocalMaster(deviceId)) {
-            scheduledExecutor.schedule(() -> configCheck(deviceId), 0, TimeUnit.SECONDS);
+            scheduledExecutor.schedule(() -> configCheck(deviceId, context), 0, TimeUnit.SECONDS);
         }
     }
 
     private void checkDevices() {
         deviceService.getAvailableDevices().forEach(device -> {
-            triggerConfigCheck(device.id());
+            triggerConfigCheck(device.id(), getContext(device.id()));
         });
     }
 
@@ -234,7 +236,7 @@
             DeviceId deviceId = event.key();
             if (event.type().equals(INSERT) || event.type().equals(UPDATE)) {
                 log.trace("Context {} for {}", event.type().name(), deviceId);
-                triggerConfigCheck(deviceId);
+                triggerConfigCheck(deviceId, event.newValue().value());
             }
         }
     }