Drop locking from FlowGroupInfoHistoryImpl

Updates here are in the fast path and we are taking locks for
something that is used ... almost never.

Remove locking and perform time-bound retries to acquire entries
instead, explaining the situation in comments.

If this is not sufficient, a TODO is also added as to what needs
to happen to make the entry acquisition reliable.

Change-Id: I21d05e074bb5033942ef4f7d0bc210a2e9bbf359
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/history/FlowGroupInfoHistoryImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/history/FlowGroupInfoHistoryImpl.java
index 6ee631a208c264199e0d7093a4a662703efe0ec5..284f1b05f97853b102be6ef5ab48b936cc86a764 100644 (file)
--- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/history/FlowGroupInfoHistoryImpl.java
+++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/history/FlowGroupInfoHistoryImpl.java
@@ -7,35 +7,56 @@
  */
 package org.opendaylight.openflowplugin.impl.device.history;
 
+import com.google.common.base.Stopwatch;
 import com.google.common.collect.EvictingQueue;
 import com.google.common.collect.ImmutableList;
 import java.util.Collection;
-import org.checkerframework.checker.lock.qual.GuardedBy;
+import java.util.ConcurrentModificationException;
+import java.util.concurrent.TimeUnit;
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.opendaylight.openflowplugin.api.openflow.FlowGroupInfo;
 import org.opendaylight.openflowplugin.api.openflow.FlowGroupInfoHistory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 @NonNullByDefault
 public final class FlowGroupInfoHistoryImpl implements FlowGroupInfoHistory, FlowGroupInfoHistoryAppender {
-    // FIXME: most of the time all we access it from the same(-ish) context. We should be able to do better. To do do
-    //        that we need an Executor which will run exclusively with the associated device
-    //        (i.e. in its Netty context) ...
-    @GuardedBy("this")
+    private static final Logger LOG = LoggerFactory.getLogger(FlowGroupInfoHistoryImpl.class);
+
     private final EvictingQueue<FlowGroupInfo> entries;
 
     public FlowGroupInfoHistoryImpl(final int size) {
         entries = EvictingQueue.create(size);
     }
 
-    // FIXME: ... in which case this method will not be synchronized ...
     @Override
-    public synchronized void appendFlowGroupInfo(final FlowGroupInfo info) {
+    public void appendFlowGroupInfo(final FlowGroupInfo info) {
+        // Always called from a specific device's context, no locking necessary
         entries.add(info);
     }
 
-    // FIXME: ... and this method will submit a request on that executor, producing a copy of the history
     @Override
-    public synchronized Collection<FlowGroupInfo> readEntries() {
+    public Collection<FlowGroupInfo> readEntries() {
+        // All of this is very optimistic and is a violation of synchronization. Since this is just a debug tool, we
+        // care about footprint the most -- and hence the appender is taking no locks whatsoever.
+        //
+        // Now if a different thread attempts to read, there is nothing to synchronize on, really, so we'll try to get a
+        // copy, potentially multiple times and afterwards we give up.
+        final Stopwatch sw = Stopwatch.createStarted();
+        do {
+            try {
+                return ImmutableList.copyOf(entries);
+            } catch (ConcurrentModificationException e) {
+                LOG.debug("Entries have been modified while we were acquiring them, retry", e);
+            }
+        } while (sw.elapsed(TimeUnit.SECONDS) < 3);
+
+        // We have failed to converge in three seconds, let's try again and fail if we fail again. That is deemed good
+        // enough for now.
+        // TODO: If this is not sufficient, we really need to have a Netty context here somehow and then offload this
+        //       access to run on that thread. Since a particular channel (and therefore DeviceContext, etc.) executes
+        //       *at most* on one thread, we'd end up either running normal operation, possibly updating entries here
+        //       *or* copy them over.
         return ImmutableList.copyOf(entries);
     }
 }