changeset 1941:1da17b854973

Allow concurrent modifications of VmStatusListener's. PR3047 Reviewed-by: jerboaa Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2016-June/019818.html
author Jie Kang <jkang@redhat.com>
date Fri, 24 Jun 2016 11:14:59 -0400
parents 6c8f7bce641f
children 9db29a97e702
files system-backend/src/main/java/com/redhat/thermostat/backend/system/VmStatusChangeNotifier.java system-backend/src/test/java/com/redhat/thermostat/backend/system/VmStatusChangeNotifierTest.java
diffstat 2 files changed, 108 insertions(+), 1 deletions(-) [+]
line wrap: on
line diff
--- a/system-backend/src/main/java/com/redhat/thermostat/backend/system/VmStatusChangeNotifier.java	Thu Jun 23 12:04:07 2016 -0400
+++ b/system-backend/src/main/java/com/redhat/thermostat/backend/system/VmStatusChangeNotifier.java	Fri Jun 24 11:14:59 2016 -0400
@@ -41,6 +41,7 @@
 import java.util.Map.Entry;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.osgi.framework.BundleContext;
 import org.osgi.framework.ServiceReference;
@@ -63,7 +64,7 @@
 
     private final Object listenerLock = new Object();
     private final Map<Integer, String> activePids;
-    private final Map<VmStatusListener, Set<Integer>> listeners = new HashMap<>();
+    private final Map<VmStatusListener, Set<Integer>> listeners = new ConcurrentHashMap<>();
 
     private final ServiceTracker tracker;
 
--- a/system-backend/src/test/java/com/redhat/thermostat/backend/system/VmStatusChangeNotifierTest.java	Thu Jun 23 12:04:07 2016 -0400
+++ b/system-backend/src/test/java/com/redhat/thermostat/backend/system/VmStatusChangeNotifierTest.java	Fri Jun 24 11:14:59 2016 -0400
@@ -36,10 +36,14 @@
 
 package com.redhat.thermostat.backend.system;
 
+import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 
+import java.util.ConcurrentModificationException;
+
 import org.junit.Test;
+import org.osgi.framework.BundleContext;
 
 import com.redhat.thermostat.agent.VmStatusListener;
 import com.redhat.thermostat.agent.VmStatusListener.Status;
@@ -96,5 +100,107 @@
         verify(listener).vmStatusChanged(Status.VM_ACTIVE, VM_ID, VM_PID);
 
     }
+    
+    /*
+     * Some backends might activate on activation of another backend. If both of
+     * them are also VmStatusListener's, a concurrent modification exception
+     * might be thrown. This tests verifies it's OK to do so (concurrent modification).
+     */
+    @Test
+    public void canAddListenersWhileFiringEvent() throws InterruptedException {
+        StubBundleContext bundleContext = new StubBundleContext();
+        VmStatusChangeNotifier notifier = new VmStatusChangeNotifier(bundleContext);
+        
+        // Add > 2 listeners. One of them registers another listener in vmStatusChanged()
+        // Thus provoking ConcurrentModificationException.
+        bundleContext.registerService(VmStatusListener.class, new TestVmStatusListener(bundleContext), null);
+        bundleContext.registerService(VmStatusListener.class, new VmStatusListener() {
+
+            @Override
+            public void vmStatusChanged(Status newStatus, String vmId,
+                    int pid) {
+                Debug.println("Second registered listener fired");
+            }
+            
+            @Override
+            public int hashCode() {
+                return 2; // second listener to be fired
+            }
+            
+        }, null);
+        bundleContext.registerService(VmStatusListener.class, new VmStatusListener() {
+
+            @Override
+            public void vmStatusChanged(Status newStatus, String vmId,
+                    int pid) {
+                Debug.println("Third registered listener fired");
+            }
+            
+            @Override
+            public int hashCode() {
+                return 3;
+            }
+            
+        }, null);
+        notifier.start();
+        
+        try {
+            notifier.notifyVmStatusChange(Status.VM_STARTED, "foo-vmid", 333);
+            // this will trigger the newly added listener being invoked and counting
+            // down the latch.
+            notifier.notifyVmStatusChange(Status.VM_STARTED, "foo-other", 9999);
+            // pass
+        } catch (ConcurrentModificationException e) {
+            fail("Unexpected conncurrent modification exception!");
+        }
+    }
+    
+    static class TestVmStatusListener implements VmStatusListener {
+
+        private final BundleContext context;
+        
+        private TestVmStatusListener(BundleContext context) {
+            this.context = context;
+        }
+        
+        @Override
+        public void vmStatusChanged(Status newStatus, String vmId, int pid) {
+            Debug.println("First registered listener fired");
+            context.registerService(VmStatusListener.class, new VmStatusListener() {
+
+                @Override
+                public void vmStatusChanged(Status newStatus, String vmId,
+                        int pid) {
+                    Debug.println("Listener registered in listener (between first and second) fired");
+                }
+                
+                @Override
+                public int hashCode() {
+                    // Pick a large hash code since that tends to result in
+                    // the listener being fired later on.
+                    return 101;
+                }
+                
+            }, null);
+        }
+        
+        @Override
+        public int hashCode() {
+            // Heuristic for HashMap it tends to be picked first when iterating over
+            return 1;
+        }
+        
+    }
+    
+    static class Debug {
+        
+        static final boolean debugOn = false; // set to true for debug output
+        
+        static void println(String msg) {
+            if (debugOn) {
+                System.err.println(msg);
+            }
+        }
+    }
 }