Fix listener sorting for partially-ordered dependencies

src/main/java/net/floodlightcontroller/core/util/ListenerDispatcher.java

@@ -18,8 +18,6 @@
 package net.floodlightcontroller.core.util;
 
 import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
 
@@ -38,6 +36,25 @@ public class ListenerDispatcher<U, T extends IListener<U>> {
     protected static Logger logger = LoggerFactory.getLogger(ListenerDispatcher.class);
     List<T> listeners = null;
     
+    private void visit(List<T> newlisteners, U type, HashSet<T> visited, 
+                       List<T> ordering, T listener) {
+        if (!visited.contains(listener)) {
+            visited.add(listener);
+            
+            for (T i : newlisteners) {
+                if (ispre(type, i, listener)) {
+                    visit(newlisteners, type, visited, ordering, i);
+                }
+            }
+            ordering.add(listener);
+        }
+    }
+    
+    private boolean ispre(U type, T l1, T l2) {
+        return (l2.isCallbackOrderingPrereq(type, l1.getName()) ||
+                l1.isCallbackOrderingPostreq(type, l2.getName()));
+    }
+    
     /**
      * Add a listener to the list of listeners
      * @param listener
@@ -48,31 +65,36 @@ public class ListenerDispatcher<U, T extends IListener<U>> {
             newlisteners.addAll(listeners);
 
         newlisteners.add(listener);
-        
-        // compute transitive closure of dependencies
-        HashSet<String> deps = new HashSet<String>();
-        for (T k : newlisteners) {
-            for (T i : newlisteners) {
-                for (T j : newlisteners) {
-                    boolean ispre = 
-                        (i.isCallbackOrderingPrereq(type, j.getName()) ||
-                         j.isCallbackOrderingPostreq(type, i.getName()));
-                    if (ispre || 
-                            (deps.contains(i.getName() + "|||" + k.getName()) &&
-                             deps.contains(k.getName() + "|||" + j.getName()))) {
-                        deps.add(i.getName() + "|||" + j.getName());
-                        // Check for dependency cycle
-                        if (deps.contains(j.getName() + "|||" + i.getName())) {
-                            logger.error("Cross dependency cycle between {} and {}",
-                                      i.getName(), j.getName());
-                        }
-                    }
+        // Find nodes without outgoing edges
+        List<T> terminals = new ArrayList<T>(); 
+        for (T i : newlisteners) {
+            boolean isterm = true;
+            for (T j : newlisteners) {
+                if (ispre(type, i, j)) {
+                    isterm = false;
+                    break;
                 }
             }
+            if (isterm) {
+                terminals.add(i);
+            }
         }
         
-        Collections.sort(newlisteners, new ListenerComparator(deps));
-        listeners = newlisteners;
+        if (terminals.size() == 0) {
+            logger.error("No listener dependency solution: " +
+            		     "No listeners without incoming dependencies");
+            listeners = newlisteners;
+            return;
+        }
+        
+        // visit depth-first traversing in the opposite order from
+        // the dependencies.  Note we will not generally detect cycles
+        HashSet<T> visited = new HashSet<T>();
+        List<T> ordering = new ArrayList<T>(); 
+        for (T term : terminals) {
+            visit(newlisteners, type, visited, ordering, term);
+        }
+        listeners = ordering;
     }
 
     /**
@@ -102,27 +124,4 @@ public class ListenerDispatcher<U, T extends IListener<U>> {
     public List<T> getOrderedListeners() {
         return listeners;
     }
-    
-    /**
-     * Comparator for listeners to use with computed transitive dependencies
-     * 
-     * @author readams
-     *
-     */
-    protected class ListenerComparator implements Comparator<IListener<U>> {
-        HashSet<String> deps;
-        
-        ListenerComparator(HashSet<String> deps) {
-            this.deps = deps;
-        }
-        
-        @Override
-        public int compare(IListener<U> arg0, IListener<U> arg1) {
-            if (deps.contains(arg0.getName() + "|||" + arg1.getName()))
-                return 1;
-            else if (deps.contains(arg1.getName() + "|||" + arg0.getName()))
-                return -1;
-            return 0;  // strictly partial ordering
-        }
-    }
 }

src/test/java/net/floodlightcontroller/core/util/MessageDispatcherTest.java

@@ -22,6 +22,10 @@ import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.replay;
 import static org.easymock.EasyMock.verify;
 
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+
 import org.junit.Test;
 import org.openflow.protocol.OFType;
 
@@ -30,57 +34,120 @@ import net.floodlightcontroller.test.FloodlightTestCase;
 
 public class MessageDispatcherTest extends FloodlightTestCase {
 
-    /**
-     * Verify that our callbacks are ordered with respect to the order specified
-     * @throws Exception
-     */
-    @Test
-    public void testCallbackOrderingBase() throws Exception {
-        testCallbackOrdering(new String[] {"topology"}, new String[] {"topology"});
-        testCallbackOrdering(new String[] {"learningswitch","topology"}, new String[] {"learningswitch","topology"});
-        testCallbackOrdering(new String[] {"topology","devicemanager","learningswitch"}, new String[] {"topology","devicemanager","learningswitch"});
-        testCallbackOrdering(new String[] {"topology","devicemanager","routing","learningswitch"}, new String[] {"topology","devicemanager","routing","learningswitch"});
-        testCallbackOrdering(new String[] {"devicemanager","topology","learningswitch","routing"}, new String[] {"topology","devicemanager","routing","learningswitch"});
+    IOFMessageListener createLMock(String name) {
+        IOFMessageListener mock = createNiceMock(IOFMessageListener.class);
+        expect(mock.getName()).andReturn(name).anyTimes();
+        return mock;
     }
-
-    protected void testCallbackOrdering(String[] addOrder, String[] verifyOrder) throws Exception {
+    
+    void addPrereqs(IOFMessageListener mock, String... deps) {
+        for (String dep : deps) {
+            expect(mock.isCallbackOrderingPrereq(OFType.PACKET_IN, dep)).andReturn(true).anyTimes();
+        }
+    }
+    
+    void testOrdering(ArrayList<IOFMessageListener> inputListeners) {
         ListenerDispatcher<OFType, IOFMessageListener> ld = 
-            new ListenerDispatcher<OFType, IOFMessageListener>();
-
-        IOFMessageListener topology = createNiceMock(IOFMessageListener.class);
-        IOFMessageListener devicemanager = createNiceMock(IOFMessageListener.class);
-        IOFMessageListener routing = createNiceMock(IOFMessageListener.class);
-        IOFMessageListener learningswitch = createNiceMock(IOFMessageListener.class);
-
-        expect(topology.getName()).andReturn("topology").anyTimes();
-        expect(devicemanager.getName()).andReturn("devicemanager").anyTimes();
-        expect(routing.getName()).andReturn("routing").anyTimes();
-        expect(learningswitch.getName()).andReturn("learningswitch").anyTimes();
-
-        expect(devicemanager.isCallbackOrderingPrereq(OFType.PACKET_IN, "topology")).andReturn(true).anyTimes();
-        expect(routing.isCallbackOrderingPrereq(OFType.PACKET_IN, "topology")).andReturn(true).anyTimes();
-        expect(routing.isCallbackOrderingPrereq(OFType.PACKET_IN, "devicemanager")).andReturn(true).anyTimes();
-        expect(learningswitch.isCallbackOrderingPrereq(OFType.PACKET_IN, "routing")).andReturn(true).anyTimes();
-        expect(learningswitch.isCallbackOrderingPrereq(OFType.PACKET_IN, "devicemanager")).andReturn(true).anyTimes();
+                new ListenerDispatcher<OFType, IOFMessageListener>();
+        
+        for (IOFMessageListener l : inputListeners) {
+            ld.addListener(OFType.PACKET_IN, l);
+        }
+        for (IOFMessageListener l : inputListeners) {
+            verify(l);
+        }
+        
+        List<IOFMessageListener> result = ld.getOrderedListeners();
+        System.out.print("Ordering: ");
+        for (IOFMessageListener l : result) {
+            System.out.print(l.getName());
+            System.out.print(",");
+        }
+        System.out.print("\n");
 
-        replay(topology, devicemanager, routing, learningswitch);
-        for (String o : addOrder) {
-            if ("topology".equals(o)) {
-                ld.addListener(OFType.PACKET_IN, topology);
-            } else if ("devicemanager".equals(o)) {
-                ld.addListener(OFType.PACKET_IN, devicemanager);
-            } else if ("routing".equals(o)) {
-                ld.addListener(OFType.PACKET_IN, routing);
-            } else {
-                ld.addListener(OFType.PACKET_IN, learningswitch);
+        for (int ind_i = 0; ind_i < result.size(); ind_i++) {
+            IOFMessageListener i = result.get(ind_i);
+            for (int ind_j = ind_i+1; ind_j < result.size(); ind_j++) {
+                IOFMessageListener j = result.get(ind_j);
+                
+                boolean orderwrong = 
+                        (i.isCallbackOrderingPrereq(OFType.PACKET_IN, j.getName()) ||
+                         j.isCallbackOrderingPostreq(OFType.PACKET_IN, i.getName()));
+                assertFalse("Invalid order: " + 
+                            ind_i + " (" + i.getName() + ") " + 
+                            ind_j + " (" + j.getName() + ") ", orderwrong);
             }
         }
+    }
+    
+    void randomTestOrdering(ArrayList<IOFMessageListener> mocks) {
+        Random rand = new Random(0);
+        ArrayList<IOFMessageListener> random = 
+                new ArrayList<IOFMessageListener>();
+        random.addAll(mocks);
+        for (int i = 0; i < 20; i++) {
+            for (int j = 0; j < random.size(); j++) {
+                int ind = rand.nextInt(mocks.size()-1);
+                IOFMessageListener tmp = random.get(j);
+                random.set(j, random.get(ind));
+                random.set(ind, tmp);
+            }
+            testOrdering(random);
+        }
+    }
+    
+    @Test
+    public void testCallbackOrderingSimple() throws Exception {
+        ArrayList<IOFMessageListener> mocks = 
+                new ArrayList<IOFMessageListener>();
+        for (int i = 0; i < 10; i++) {
+            mocks.add(createLMock(""+i));
+        }
+        for (int i = 1; i < 10; i++) {
+            addPrereqs(mocks.get(i), ""+(i-1));
+        }
+        for (IOFMessageListener l : mocks) {
+            replay(l);
+        }
+        randomTestOrdering(mocks);
+    }
 
-        verify(topology, devicemanager, routing, learningswitch);
-        for (int i = 0; i < verifyOrder.length; ++i) {
-            String o = verifyOrder[i];
-            assertEquals(o, ld.getOrderedListeners().get(i).getName());
+    @Test
+    public void testCallbackOrderingPartial() throws Exception {
+        ArrayList<IOFMessageListener> mocks = 
+                new ArrayList<IOFMessageListener>();
+        for (int i = 0; i < 10; i++) {
+            mocks.add(createLMock(""+i));
+        }
+        for (int i = 1; i < 5; i++) {
+            addPrereqs(mocks.get(i), ""+(i-1));
+        }
+        for (int i = 6; i < 10; i++) {
+            addPrereqs(mocks.get(i), ""+(i-1));
         }
+        for (IOFMessageListener l : mocks) {
+            replay(l);
+        }
+        randomTestOrdering(mocks);
     }
+    
 
+    @Test
+    public void testCallbackOrderingPartial2() throws Exception {
+        ArrayList<IOFMessageListener> mocks = 
+                new ArrayList<IOFMessageListener>();
+        for (int i = 0; i < 10; i++) {
+            mocks.add(createLMock(""+i));
+        }
+        for (int i = 2; i < 5; i++) {
+            addPrereqs(mocks.get(i), ""+(i-1));
+        }
+        for (int i = 6; i < 9; i++) {
+            addPrereqs(mocks.get(i), ""+(i-1));
+        }
+        for (IOFMessageListener l : mocks) {
+            replay(l);
+        }
+        randomTestOrdering(mocks);
+    }
 }