Fix LearnCodecUtil scalability

LearnCodecUtil.buildFlowModSpecs() is statically synchronized to
keep track of overflows. Encapsulate that state in an object,
disarming this particular landmine.

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

diff --git a/extension/openflowjava-extension-nicira/src/main/java/org/opendaylight/openflowjava/nx/codec/action/LearnCodecUtil.java b/extension/openflowjava-extension-nicira/src/main/java/org/opendaylight/openflowjava/nx/codec/action/LearnCodecUtil.java
index b5a188eaf5b901993631f830eaf5f584e6d16415..2e9e10f14533957850cbfc169caaf2ef88730827 100644 (file)
--- a/extension/openflowjava-extension-nicira/src/main/java/org/opendaylight/openflowjava/nx/codec/action/LearnCodecUtil.java
+++ b/extension/openflowjava-extension-nicira/src/main/java/org/opendaylight/openflowjava/nx/codec/action/LearnCodecUtil.java
@@ -59,12 +59,12 @@ public final class LearnCodecUtil {
     private static final int TO_PORT_LENGTH = 8;
     private static final int EMPTY_FLOW_MOD_LENGTH = 2;
 
-    // FIXME: You might wonder, why has a static utility method a static mutable field. Look for FIXMEs and users of
-    //        this field for a nice story.
-    private static short length;
+    // For overflow detection. We should probably just ByteBuf.slice() and fail-fast, i.e. don't read stuff beyond
+    // message length limit.
+    private short length;
 
-    private LearnCodecUtil() {
-        // Hidden on purpose
+    private LearnCodecUtil(final short length) {
+        this.length = length;
     }
 
     static short deserializeHeader(final ByteBuf message) {
@@ -202,14 +202,14 @@ public final class LearnCodecUtil {
             .setFinHardTimeout(readUint16(message));
     }
 
-    // FIXME: OMG: this thing has its synchronized global state ...
-    static synchronized void buildFlowModSpecs(final NxActionLearnBuilder nxActionLearnBuilder, final ByteBuf message,
+    static void buildFlowModSpecs(final NxActionLearnBuilder nxActionLearnBuilder, final ByteBuf message,
             final short messageLength) {
-        // ... which is this integer!
-        LearnCodecUtil.length = messageLength;
+        new LearnCodecUtil(messageLength).buildFlowModSpecs(nxActionLearnBuilder, message);
+    }
 
+    private void buildFlowModSpecs(final NxActionLearnBuilder nxActionLearnBuilder, final ByteBuf message) {
         final List<FlowMods> flowModeList = new ArrayList<>();
-        while (LearnCodecUtil.length > 0) {
+        while (length > 0) {
             final FlowMods flowMod = readFlowMod(message);
             if (flowMod != null) {
                 flowModeList.add(flowMod);
@@ -218,15 +218,14 @@ public final class LearnCodecUtil {
             }
         }
 
-        // ... which is used for this warning
-        if (LearnCodecUtil.length != 0) {
+        if (length != 0) {
             LOG.error("Learn Codec read {} bytes more than needed from stream. Packet might be corrupted",
-                    Math.abs(messageLength));
+                Math.abs(length));
         }
         nxActionLearnBuilder.setFlowMods(flowModeList);
     }
 
-    private static FlowMods readFlowMod(final ByteBuf message) {
+    private FlowMods readFlowMod(final ByteBuf message) {
         final short header = message.readShort();
         length -= Short.BYTES;
         if (header == 0) {
@@ -254,7 +253,7 @@ public final class LearnCodecUtil {
         return null;
     }
 
-    private static FlowMods readFlowModAddMatchFromField(final ByteBuf message, final short numBits) {
+    private FlowMods readFlowModAddMatchFromField(final ByteBuf message, final short numBits) {
         final var builder = new FlowModAddMatchFromFieldBuilder()
             .setSrcField(readUint32(message))
             .setSrcOfs((int) message.readShort())
@@ -270,7 +269,7 @@ public final class LearnCodecUtil {
             .build();
     }
 
-    private static FlowMods readFlowModAddMatchFromValue(final ByteBuf message, final short numBits) {
+    private FlowMods readFlowModAddMatchFromValue(final ByteBuf message, final short numBits) {
         final var builder = new FlowModAddMatchFromValueBuilder()
             .setValue(readUint16(message))
             .setSrcField(readUint32(message))
@@ -285,7 +284,7 @@ public final class LearnCodecUtil {
             .build();
     }
 
-    private static FlowMods readFlowModCopyFromField(final ByteBuf message, final short numBits) {
+    private FlowMods readFlowModCopyFromField(final ByteBuf message, final short numBits) {
         final var builder = new FlowModCopyFieldIntoFieldBuilder()
             .setSrcField(readUint32(message))
             .setSrcOfs((int) message.readShort())
@@ -301,7 +300,7 @@ public final class LearnCodecUtil {
             .build();
     }
 
-    private static FlowMods readFlowModCopyFromValue(final ByteBuf message, final short numBits) {
+    private FlowMods readFlowModCopyFromValue(final ByteBuf message, final short numBits) {
         final var builder = new FlowModCopyValueIntoFieldBuilder()
             .setValue(readUint16(message))
             .setDstField(readUint32(message))
@@ -316,7 +315,7 @@ public final class LearnCodecUtil {
             .build();
     }
 
-    private static FlowMods readFlowToPort(final ByteBuf message, final short numBits) {
+    private FlowMods readFlowToPort(final ByteBuf message, final short numBits) {
         final var builder = new FlowModOutputToPortBuilder()
             .setSrcField(readUint32(message))
             .setSrcOfs((int) message.readShort())