Make Device (more) thread-safe

* Make all fields final or volatile
* Still not 100% safe (visibility): lastSeenTimestamp of the entities
  can be changed unsynchronized

src/main/java/net/floodlightcontroller/devicemanager/internal/Device.java

@@ -53,26 +53,26 @@ public class Device implements IDevice {
     protected static Logger log =
             LoggerFactory.getLogger(Device.class);
 
-    protected Long deviceKey;
-    protected DeviceManagerImpl deviceManager;
+    private final Long deviceKey;
+    protected final DeviceManagerImpl deviceManager;
 
-    protected Entity[] entities;
-    protected IEntityClass entityClass;
+    protected final Entity[] entities;
+    private final IEntityClass entityClass;
 
-    protected String macAddressString;
+    protected final String macAddressString;
     // the vlan Ids from the entities of this device
-    protected Short[] vlanIds;
-    protected String dhcpClientName;
+    protected final Short[] vlanIds;
+    protected volatile String dhcpClientName;
 
     /**
      * These are the old attachment points for the device that were
      * valid no more than INACTIVITY_TIME ago.
      */
-    protected List<AttachmentPoint> oldAPs;
+    protected volatile List<AttachmentPoint> oldAPs;
     /**
      * The current attachment points for the device.
      */
-    protected List<AttachmentPoint> attachmentPoints;
+    protected volatile List<AttachmentPoint> attachmentPoints;
 
     // ************
     // Constructors
@@ -115,7 +115,7 @@ public class Device implements IDevice {
                 this.attachmentPoints.add(ap);
             }
         }
-        computeVlandIds();
+        vlanIds = computeVlandIds();
     }
 
     /**
@@ -150,7 +150,7 @@ public class Device implements IDevice {
                 HexString.toHexString(this.entities[0].getMacAddress(), 6);
         this.entityClass = entityClass;
         Arrays.sort(this.entities);
-        computeVlandIds();
+        vlanIds = computeVlandIds();
     }
 
     /**
@@ -213,17 +213,16 @@ public class Device implements IDevice {
                 HexString.toHexString(this.entities[0].getMacAddress(), 6);
 
         this.entityClass = device.entityClass;
-        computeVlandIds();
+        vlanIds = computeVlandIds();
     }
 
-    private void computeVlandIds() {
+    private Short[]  computeVlandIds() {
         if (entities.length == 1) {
             if (entities[0].getVlan() != null) {
-                vlanIds = new Short[]{ entities[0].getVlan() };
+                return new Short[]{ entities[0].getVlan() };
             } else {
-                vlanIds = new Short[] { Short.valueOf((short)-1) };
+                return new Short[] { Short.valueOf((short)-1) };
             }
-            return;
         }
 
         TreeSet<Short> vals = new TreeSet<Short>();
@@ -233,7 +232,7 @@ public class Device implements IDevice {
             else
                 vals.add(e.getVlan());
         }
-        vlanIds = vals.toArray(new Short[vals.size()]);
+        return vals.toArray(new Short[vals.size()]);
     }
 
     /**

src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java

@@ -1920,7 +1920,7 @@ IFlowReconcileListener, IInfoProvider {
 
                 debugCounters.updateCounter(CNT_ENTITY_REMOVED_TIMEOUT);
                 for (Entity e : toRemove) {
-                    removeEntity(e, d.getEntityClass(), d.deviceKey, toKeep);
+                    removeEntity(e, d.getEntityClass(), d.getDeviceKey(), toKeep);
                 }
 
                 if (toKeep.size() > 0) {
@@ -1929,7 +1929,7 @@ IFlowReconcileListener, IInfoProvider {
                                                       d.oldAPs,
                                                       d.attachmentPoints,
                                                       toKeep,
-                                                      d.entityClass);
+                                                      d.getEntityClass());
 
                     EnumSet<DeviceField> changedFields =
                             EnumSet.noneOf(DeviceField.class);

src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java

@@ -352,7 +352,7 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
         assertSame(d1, deviceManager.learnDeviceByEntity(entity1));
         assertSame(d1, deviceManager.findDeviceByEntity(entity1));
         assertEquals(DefaultEntityClassifier.entityClass ,
-                          d1.entityClass);
+                          d1.getEntityClass());
         assertArrayEquals(new Short[] { -1 }, d1.getVlanId());
         assertArrayEquals(new Integer[] { }, d1.getIPv4Addresses());
 
@@ -367,7 +367,7 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
         assertFalse(d1.equals(d2));
         assertNotSame(d1, d2);
         assertNotSame(d1.getDeviceKey(), d2.getDeviceKey());
-        assertEquals(MockEntityClassifier.testEC, d2.entityClass);
+        assertEquals(MockEntityClassifier.testEC, d2.getEntityClass());
         assertArrayEquals(new Short[] { -1 }, d2.getVlanId());
         assertArrayEquals(new Integer[] { }, d2.getIPv4Addresses());
 
@@ -381,7 +381,7 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
         Device d3 = deviceManager.learnDeviceByEntity(entity3);
         assertNotSame(d2, d3);
         assertEquals(d2.getDeviceKey(), d3.getDeviceKey());
-        assertEquals(MockEntityClassifier.testEC, d3.entityClass);
+        assertEquals(MockEntityClassifier.testEC, d3.getEntityClass());
         assertArrayEquals(new Integer[] { 1 },
                           d3.getIPv4Addresses());
         assertArrayEquals(new SwitchPort[] { new SwitchPort(10L, 1) },
@@ -401,7 +401,7 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
         Device d4 = deviceManager.learnDeviceByEntity(entity4);
         assertNotSame(d1, d4);
         assertEquals(d1.getDeviceKey(), d4.getDeviceKey());
-        assertEquals(DefaultEntityClassifier.entityClass, d4.entityClass);
+        assertEquals(DefaultEntityClassifier.entityClass, d4.getEntityClass());
         assertArrayEquals(new Integer[] { 1 },
                           d4.getIPv4Addresses());
         assertArrayEquals(new SwitchPort[] { new SwitchPort(1L, 1) },
@@ -1306,24 +1306,21 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
                     // is different enough to compare differently in equals
                     // but otherwise looks the same.
                     // It's ugly but it works.
-                    Entity[] curEntities = new Entity[d.getEntities().length];
-                    int i = 0;
                     // clone entities
+                    Device newDevice = d;
                     for (Entity e: d.getEntities()) {
-                        curEntities[i] = new Entity (e.macAddress,
-                                                     e.vlan,
-                                                     e.ipv4Address,
-                                                     e.switchDPID,
-                                                     e.switchPort,
-                                                     e.lastSeenTimestamp);
+                        Entity newEntity = new Entity (e.macAddress,
+                                                       e.vlan,
+                                                       e.ipv4Address,
+                                                       e.switchDPID,
+                                                       e.switchPort,
+                                                       e.lastSeenTimestamp);
                         if (e.vlan == null)
-                            curEntities[i].vlan = (short)1;
+                            newEntity.vlan = (short)1;
                         else
-                            curEntities[i].vlan = (short)((e.vlan + 1 % 4095)+1);
-                        i++;
+                             newEntity.vlan = (short)((e.vlan + 1 % 4095)+1);
+                        newDevice = new Device(newDevice, newEntity, -1);
                     }
-                    Device newDevice = new Device(d, curEntities[0], -1);
-                    newDevice.entities = curEntities;
                     assertEquals(false, newDevice.equals(d));
                     super.put(newDevice.getDeviceKey(), newDevice);
                 }