Do not use Strings for internal messages 10/37010/5
authorRobert Varga <rovarga@cisco.com>
Fri, 1 Apr 2016 14:14:25 +0000 (16:14 +0200)
committerRobert Varga <rovarga@cisco.com>
Sat, 2 Apr 2016 13:22:51 +0000 (15:22 +0200)
Using a String for the message identity has the effect of the equality
running on Strings, which has the effect that message is not really
isolated, but can be sent by anyone who knows the message content.

Instantiate simple Objects instead, which force a proper identity
equality check. Retain debuggability by having these instances override
toString().

Change-Id: Ia581e0a1e023ace10b5dbb81a44092ca04a4ff8f
Signed-off-by: Robert Varga <rovarga@cisco.com>
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorServerConfigurationSupportTest.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/Shard.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTest.java

index 8b68711..d33f780 100644 (file)
@@ -29,7 +29,12 @@ import scala.concurrent.duration.Duration;
  * @author Thomas Pantelis
  */
 class RaftActorSnapshotMessageSupport {
-    static final String COMMIT_SNAPSHOT = "commit_snapshot";
+    static final Object COMMIT_SNAPSHOT = new Object() {
+        @Override
+        public String toString() {
+            return "commit_snapshot";
+        }
+    };
 
     private final RaftActorContext context;
     private final RaftActorSnapshotCohort cohort;
@@ -69,7 +74,7 @@ class RaftActorSnapshotMessageSupport {
             onSaveSnapshotFailure((SaveSnapshotFailure) message);
         } else if (message instanceof CaptureSnapshotReply) {
             onCaptureSnapshotReply(((CaptureSnapshotReply) message).getSnapshot());
-        } else if (message.equals(COMMIT_SNAPSHOT)) {
+        } else if (COMMIT_SNAPSHOT.equals(message)) {
             context.getSnapshotManager().commit(-1);
         } else if (message instanceof GetSnapshot) {
             onGetSnapshot(sender);
index 514346f..ec5f024 100644 (file)
@@ -80,6 +80,7 @@ public class RaftActorServerConfigurationSupportTest extends AbstractActorTest {
     static final String NEW_SERVER_ID = "new-server";
     static final String NEW_SERVER_ID2 = "new-server2";
     private static final Logger LOG = LoggerFactory.getLogger(RaftActorServerConfigurationSupportTest.class);
+    private static final Class<?> COMMIT_MESSAGE_CLASS = RaftActorSnapshotMessageSupport.COMMIT_SNAPSHOT.getClass();
     private static final boolean NO_PERSISTENCE = false;
     private static final boolean PERSISTENT = true;
 
@@ -436,7 +437,7 @@ public class RaftActorServerConfigurationSupportTest extends AbstractActorTest {
 
         leaderActor.tell(new InitiateCaptureSnapshot(), leaderActor);
 
-        String commitMsg = expectFirstMatching(leaderCollectorActor, String.class);
+        Object commitMsg = expectFirstMatching(leaderCollectorActor, COMMIT_MESSAGE_CLASS);
 
         leaderActor.tell(new AddServer(NEW_SERVER_ID, newFollowerRaftActor.path().toString(), true), testKit.getRef());
 
@@ -479,7 +480,7 @@ public class RaftActorServerConfigurationSupportTest extends AbstractActorTest {
         ((DefaultConfigParamsImpl)leaderActorContext.getConfigParams()).setElectionTimeoutFactor(1);
 
         // Drop commit message so the snapshot doesn't complete.
-        leaderRaftActor.setDropMessageOfType(String.class);
+        leaderRaftActor.setDropMessageOfType(COMMIT_MESSAGE_CLASS);
 
         leaderActor.tell(new InitiateCaptureSnapshot(), leaderActor);
 
@@ -512,13 +513,13 @@ public class RaftActorServerConfigurationSupportTest extends AbstractActorTest {
         TestActorRef<MessageCollectorActor> leaderCollectorActor = newLeaderCollectorActor(leaderRaftActor);
 
         // Drop the commit message so the snapshot doesn't complete yet.
-        leaderRaftActor.setDropMessageOfType(String.class);
+        leaderRaftActor.setDropMessageOfType(COMMIT_MESSAGE_CLASS);
 
         leaderActor.tell(new InitiateCaptureSnapshot(), leaderActor);
 
         leaderActor.tell(new AddServer(NEW_SERVER_ID, newFollowerRaftActor.path().toString(), true), testKit.getRef());
 
-        String commitMsg = expectFirstMatching(leaderCollectorActor, String.class);
+        Object commitMsg = expectFirstMatching(leaderCollectorActor, COMMIT_MESSAGE_CLASS);
 
         // Change the leader behavior to follower
         leaderActor.tell(new Follower(leaderActorContext), leaderActor);
index a47e437..a647554 100644 (file)
@@ -82,11 +82,21 @@ import scala.concurrent.duration.FiniteDuration;
  * </p>
  */
 public class Shard extends RaftActor {
-
-    protected static final Object TX_COMMIT_TIMEOUT_CHECK_MESSAGE = "txCommitTimeoutCheck";
+    @VisibleForTesting
+    static final Object TX_COMMIT_TIMEOUT_CHECK_MESSAGE = new Object() {
+        @Override
+        public String toString() {
+            return "txCommitTimeoutCheck";
+        }
+    };
 
     @VisibleForTesting
-    static final Object GET_SHARD_MBEAN_MESSAGE = "getShardMBeanMessage";
+    static final Object GET_SHARD_MBEAN_MESSAGE = new Object() {
+        @Override
+        public String toString() {
+            return "getShardMBeanMessage";
+        }
+    };
 
     // FIXME: shard names should be encapsulated in their own class and this should be exposed as a constant.
     public static final String DEFAULT_NAME = "default";
index 5118de4..2da7e5e 100644 (file)
@@ -28,7 +28,12 @@ import scala.concurrent.duration.FiniteDuration;
  * @author Thomas Pantelis
  */
 class EntityOwnershipShardCommitCoordinator {
-    private static final Object COMMIT_RETRY_MESSAGE = "entityCommitRetry";
+    private static final Object COMMIT_RETRY_MESSAGE = new Object() {
+        @Override
+        public String toString() {
+            return "entityCommitRetry";
+        }
+    };
 
     private final Logger log;
     private int transactionIDCounter = 0;
@@ -50,7 +55,7 @@ class EntityOwnershipShardCommitCoordinator {
         } else if(message instanceof akka.actor.Status.Failure) {
             // Failure reply from a local commit.
             inflightCommitFailure(((Failure)message).cause(), shard);
-        } else if(message.equals(COMMIT_RETRY_MESSAGE)) {
+        } else if(COMMIT_RETRY_MESSAGE.equals(message)) {
             retryInflightCommit(shard);
         } else {
             handled = false;
index 407cc60..d743e79 100644 (file)
@@ -2013,7 +2013,7 @@ public class ShardTest extends AbstractShardTest {
                         @Override
                         public void handleCommand(final Object message) {
                             super.handleCommand(message);
-                            if(message.equals(TX_COMMIT_TIMEOUT_CHECK_MESSAGE)) {
+                            if(TX_COMMIT_TIMEOUT_CHECK_MESSAGE.equals(message)) {
                                 if(cleaupCheckLatch.get() != null) {
                                     cleaupCheckLatch.get().countDown();
                                 }
@@ -2110,7 +2110,8 @@ public class ShardTest extends AbstractShardTest {
                 public void handleCommand(final Object message) {
                     super.handleCommand(message);
 
-                    if (message instanceof SaveSnapshotSuccess || message.equals("commit_snapshot")) {
+                    // XXX:  commit_snapshot equality check references RaftActorSnapshotMessageSupport.COMMIT_SNAPSHOT
+                    if (message instanceof SaveSnapshotSuccess || "commit_snapshot".equals(message.toString())) {
                         latch.get().countDown();
                     }
                 }