Fix for IOFSwitch/OFSwitchImpl leak
- Fix for ONOS-1765
- OFSwitchImpl.java
Changes TLS bufffer map to WeakHashMap to eliminate buffer for
eliminated switches.
- OFMessageDamper.java
Added a hack to remove cached entries related to disconnected switch
- TimedCache.java
Backdoor acccess to manually invalidate cached entries
- FlowProgrammer.java
Propagate removeSwitch event
FlowPusher -> OFMessageDamper to invalidate cache.
- FlowPusher.java
Switched `assignedQueues` to concurrent version of WeakHashMap
- PlanInstallRuntime.java
Changed FlowModCount map to WeakHashMap and removed
strong reference to IOFSwitch
Change-Id: Idb5014379ebc5658d0ae58ebcdbb2bf03e981df7
diff --git a/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java b/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java
index ce865d8..0bb9f2f 100644
--- a/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java
+++ b/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java
@@ -23,10 +23,10 @@
import java.util.Collection;
import java.util.Collections;
import java.util.Date;
-import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
+import java.util.WeakHashMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.Future;
@@ -135,7 +135,7 @@
new ThreadLocal<Map<OFSwitchImpl, List<OFMessage>>>() {
@Override
protected Map<OFSwitchImpl, List<OFMessage>> initialValue() {
- return new HashMap<OFSwitchImpl, List<OFMessage>>();
+ return new WeakHashMap<OFSwitchImpl, List<OFMessage>>();
}
};
diff --git a/src/main/java/net/floodlightcontroller/util/OFMessageDamper.java b/src/main/java/net/floodlightcontroller/util/OFMessageDamper.java
index 47a92a9..5faf38e 100644
--- a/src/main/java/net/floodlightcontroller/util/OFMessageDamper.java
+++ b/src/main/java/net/floodlightcontroller/util/OFMessageDamper.java
@@ -6,7 +6,10 @@
import java.io.IOException;
import java.util.EnumSet;
+import java.util.Iterator;
import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
import net.floodlightcontroller.core.FloodlightContext;
import net.floodlightcontroller.core.IOFSwitch;
@@ -81,6 +84,8 @@
TimedCache<DamperEntry> cache;
EnumSet<OFType> msgTypesToCache;
+ // executor for invalidate task
+ private static ExecutorService executor = Executors.newFixedThreadPool(1);
/**
* @param capacity the maximum number of messages that should be
@@ -147,4 +152,24 @@
return true;
}
}
+
+ /**
+ * Invalidates all the damper cache entries for the specified switch.
+ *
+ * @param sw switch connection to invalidate
+ */
+ public void invalidate(final IOFSwitch sw) {
+ executor.submit(new Runnable() {
+ @Override
+ public void run() {
+ Iterator<DamperEntry> it = cache.getCachedEntries().iterator();
+ while (it.hasNext()) {
+ DamperEntry entry = it.next();
+ if (entry.sw == sw) {
+ it.remove();
+ }
+ }
+ }
+ });
+ }
}
diff --git a/src/main/java/net/floodlightcontroller/util/TimedCache.java b/src/main/java/net/floodlightcontroller/util/TimedCache.java
index 2a90f0b..76f1e57 100644
--- a/src/main/java/net/floodlightcontroller/util/TimedCache.java
+++ b/src/main/java/net/floodlightcontroller/util/TimedCache.java
@@ -17,6 +17,7 @@
package net.floodlightcontroller.util;
+import java.util.Set;
import java.util.concurrent.ConcurrentMap;
import com.googlecode.concurrentlinkedhashmap.ConcurrentLinkedHashMap;
@@ -60,7 +61,7 @@
* @return boolean
*/
public boolean update(K key) {
- Long curr = new Long(System.currentTimeMillis());
+ long curr = System.currentTimeMillis();
Long prev = cache.putIfAbsent(key, curr);
if (prev == null) {
@@ -75,4 +76,15 @@
return true;
}
+
+ /**
+ * Gets all the cached entries.
+ * <p/>
+ * Modification to the returned set will be reflected to the cache.
+ *
+ * @return cached entries
+ */
+ Set<K> getCachedEntries() {
+ return cache.keySet();
+ }
}
diff --git a/src/main/java/net/onrc/onos/core/flowprogrammer/FlowProgrammer.java b/src/main/java/net/onrc/onos/core/flowprogrammer/FlowProgrammer.java
index 838533d..046d130 100644
--- a/src/main/java/net/onrc/onos/core/flowprogrammer/FlowProgrammer.java
+++ b/src/main/java/net/onrc/onos/core/flowprogrammer/FlowProgrammer.java
@@ -128,6 +128,7 @@
synchronizer.interrupt(sw);
}
pusher.deleteQueue(sw, true);
+ pusher.invalidate(sw);
}
@Override
diff --git a/src/main/java/net/onrc/onos/core/flowprogrammer/FlowPusher.java b/src/main/java/net/onrc/onos/core/flowprogrammer/FlowPusher.java
index 98c2f97..37e1b44 100644
--- a/src/main/java/net/onrc/onos/core/flowprogrammer/FlowPusher.java
+++ b/src/main/java/net/onrc/onos/core/flowprogrammer/FlowPusher.java
@@ -70,6 +70,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.cache.CacheBuilder;
+
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
/**
@@ -299,8 +301,11 @@
* Main thread that reads messages from queues and sends them to switches.
*/
private class FlowPusherThread extends Thread {
+ // Weak ConncurrentHashMap
private Map<IOFSwitch, SwitchQueue> assignedQueues
- = new ConcurrentHashMap<IOFSwitch, SwitchQueue>();
+ = CacheBuilder.newBuilder()
+ .weakKeys()
+ .<IOFSwitch, SwitchQueue>build().asMap();
final Lock queuingLock = new ReentrantLock();
final Condition messagePushed = queuingLock.newCondition();
@@ -612,6 +617,17 @@
}
}
+ /**
+ * Invalidate.
+ *
+ * @param sw switch
+ *
+ * @see OFMessageDamper#invalidate(IOFSwitch)
+ */
+ public void invalidate(IOFSwitch sw) {
+ messageDamper.invalidate(sw);
+ }
+
@Override
public boolean add(IOFSwitch sw, OFMessage msg) {
return add(sw, msg, MsgPriority.NORMAL);
diff --git a/src/main/java/net/onrc/onos/core/intent/runtime/PlanInstallRuntime.java b/src/main/java/net/onrc/onos/core/intent/runtime/PlanInstallRuntime.java
index bd332ac..f942fa8 100644
--- a/src/main/java/net/onrc/onos/core/intent/runtime/PlanInstallRuntime.java
+++ b/src/main/java/net/onrc/onos/core/intent/runtime/PlanInstallRuntime.java
@@ -1,11 +1,12 @@
package net.onrc.onos.core.intent.runtime;
+import java.lang.ref.WeakReference;
import java.util.ArrayList;
-import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.WeakHashMap;
import java.util.concurrent.ExecutionException;
import net.floodlightcontroller.core.IFloodlightProviderService;
@@ -50,7 +51,7 @@
* TODO: This class should be wrapped into a more generic debugging framework when available.
*/
private static class FlowModCount {
- IOFSwitch sw;
+ WeakReference<IOFSwitch> sw;
long modFlows = 0;
long delFlows = 0;
long errors = 0;
@@ -61,7 +62,7 @@
* @param sw the switch for FlowMod statistics collection
*/
FlowModCount(IOFSwitch sw) {
- this.sw = sw;
+ this.sw = new WeakReference<>(sw);
}
/**
@@ -92,10 +93,12 @@
*/
@Override
public String toString() {
- return "sw:" + sw.getStringId() + ": modify " + modFlows + " delete " + delFlows + " error " + errors;
+ final IOFSwitch swTemp = sw.get();
+ return "sw:" + ((swTemp == null) ? "disconnected" : swTemp.getStringId())
+ + ": modify " + modFlows + " delete " + delFlows + " error " + errors;
}
- static Map<IOFSwitch, FlowModCount> map = new HashMap<>();
+ static Map<IOFSwitch, FlowModCount> map = new WeakHashMap<>();
/**
* This function is used for collecting statistics information. It should be called for