From 01c2f312ef169509e581df4fb72864c8ffe37e8e Mon Sep 17 00:00:00 2001
From: Gregor Maier <gregor.maier@bigswitch.com>
Date: Mon, 13 May 2013 11:02:28 -0700
Subject: [PATCH] Make Device (more) thread-safe
* Make all fields final or volatile
* Still not 100% safe (visibility): lastSeenTimestamp of the entities
can be changed unsynchronized
---
.../devicemanager/internal/Device.java | 33 +++++++++----------
.../internal/DeviceManagerImpl.java | 4 +--
.../internal/DeviceManagerImplTest.java | 31 ++++++++---------
3 files changed, 32 insertions(+), 36 deletions(-)
diff --git a/src/main/java/net/floodlightcontroller/devicemanager/internal/Device.java b/src/main/java/net/floodlightcontroller/devicemanager/internal/Device.java
index e7e9a4509..457bb5644 100755
--- a/src/main/java/net/floodlightcontroller/devicemanager/internal/Device.java
+++ b/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()]);
}
/**
diff --git a/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java b/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java
index d15e43f28..8586501d4 100755
--- a/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java
+++ b/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);
diff --git a/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java b/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java
index f363ef1be..dc23400ac 100644
--- a/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java
+++ b/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);
}
--
GitLab