[geode] 02/02: fix a race: FindRemoteTXMessageReply should not modify tx commit message state as it does not hold the tx lock; instead, clinet version is modified in TXRemoteCommitMessage when the tx lock is held.

[es...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

eshu11 pushed a commit to branch feature/GEODE-4142
in repository https://gitbox.apache.org/repos/asf/geode.git

commit 5828c00f6d67066882a5edd913735eaa42203936
Author: eshu <es...@pivotal.io>
AuthorDate: Mon Jan 22 13:10:03 2018 -0800

    fix a race: FindRemoteTXMessageReply should not modify tx commit message state as it does not hold the tx lock;
    instead, clinet version is modified in TXRemoteCommitMessage when the tx lock is held.
---
 .../geode/internal/cache/FindRemoteTXMessage.java  | 12 ++---
 .../internal/cache/JtaAfterCompletionMessage.java  |  5 +-
 .../internal/cache/TXRemoteCommitMessage.java      |  3 ++
 .../sockets/command/TXSynchronizationCommand.java  |  2 +-
 .../internal/cache/TXRemoteCommitMessageTest.java  | 54 ++++++++++++++++++++++
 .../codeAnalysis/sanctionedDataSerializables.txt   |  4 +-
 6 files changed, 68 insertions(+), 12 deletions(-)

diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/FindRemoteTXMessage.java b/geode-core/src/main/java/org/apache/geode/internal/cache/FindRemoteTXMessage.java
index bedd04d..7afb9db 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/FindRemoteTXMessage.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/FindRemoteTXMessage.java
@@ -255,14 +255,12 @@ public class FindRemoteTXMessage extends HighPriorityDistributionMessage
     public void toData(DataOutput out) throws IOException {
       super.toData(out);
       out.writeBoolean(this.isHostingTx);
-      boolean sendTXCommitMessage = this.txCommitMessage != null;
+      // Do not send TxCommitMessage for TXFailover.
+      // FindRemoteTXMessage does not hold the tx lock and should not
+      // modify TXCommitMessage state to cause race condition.
+      // This will force a retry and use the TXRemoteCommitMessage to get the commit message.
+      boolean sendTXCommitMessage = false;
       out.writeBoolean(sendTXCommitMessage);
-      if (sendTXCommitMessage) {
-        out.writeBoolean(this.isPartialCommitMessage);
-        // since this message is going to a peer, reset client version
-        txCommitMessage.setClientVersion(null); // fixes bug 46529
-        InternalDataSerializer.writeDSFID(txCommitMessage, out);
-      }
     }
 
     @Override
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/JtaAfterCompletionMessage.java b/geode-core/src/main/java/org/apache/geode/internal/cache/JtaAfterCompletionMessage.java
index d843ba8..96b8d33 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/JtaAfterCompletionMessage.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/JtaAfterCompletionMessage.java
@@ -80,8 +80,9 @@ public class JtaAfterCompletionMessage extends TXMessage {
     }
     TXCommitMessage commitMessage = txMgr.getRecentlyCompletedMessage(txId);
     if (commitMessage != null) {
-      TXRemoteCommitReplyMessage.send(getSender(), getProcessorId(), commitMessage,
-          getReplySender(dm));
+      TXCommitMessage message =
+          commitMessage == TXCommitMessage.ROLLBACK_MSG ? null : commitMessage;
+      TXRemoteCommitReplyMessage.send(getSender(), getProcessorId(), message, getReplySender(dm));
       return false;
     }
     TXStateProxy txState = txMgr.getTXState();
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/TXRemoteCommitMessage.java b/geode-core/src/main/java/org/apache/geode/internal/cache/TXRemoteCommitMessage.java
index d9d5cb8..7960fb2 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/TXRemoteCommitMessage.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/TXRemoteCommitMessage.java
@@ -171,6 +171,9 @@ public class TXRemoteCommitMessage extends TXMessage {
     public static void send(InternalDistributedMember recipient, int processorId,
         TXCommitMessage val, ReplySender replySender) throws RemoteOperationException {
       Assert.assertTrue(recipient != null, "TXRemoteCommitReply NULL reply message");
+      if (val != null) {
+        val.setClientVersion(null);
+      }
       TXRemoteCommitReplyMessage m = new TXRemoteCommitReplyMessage(processorId, val);
       m.setRecipient(recipient);
       replySender.putOutgoing(m);
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
index c1fb616..2671e4e 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/TXSynchronizationCommand.java
@@ -96,7 +96,7 @@ public class TXSynchronizationCommand extends BaseCommand {
 
     final TXId txId = txProxy.getTxId();
     TXCommitMessage commitMessage = txMgr.getRecentlyCompletedMessage(txId);
-    if (commitMessage != null) {
+    if (commitMessage != null && commitMessage != TXCommitMessage.ROLLBACK_MSG) {
       assert type == CompletionType.AFTER_COMPLETION;
       try {
         CommitCommand.writeCommitResponse(commitMessage, clientMessage, serverConnection);
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/TXRemoteCommitMessageTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/TXRemoteCommitMessageTest.java
new file mode 100644
index 0000000..2724499
--- /dev/null
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/TXRemoteCommitMessageTest.java
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.internal.cache;
+
+import static org.junit.Assert.*;
+
+import java.io.DataInput;
+import java.io.DataOutputStream;
+
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.internal.ByteArrayData;
+import org.apache.geode.test.junit.categories.UnitTest;
+
+@Category(UnitTest.class)
+public class TXRemoteCommitMessageTest {
+
+  @Test
+  public void testFindRemoteTXMessageReplyDoesNotSendCommitMessage() throws Exception {
+    FindRemoteTXMessage.FindRemoteTXMessageReply findRemoteTXMessageReply =
+        new FindRemoteTXMessage.FindRemoteTXMessageReply();
+    findRemoteTXMessageReply.isHostingTx = true;
+    findRemoteTXMessageReply.txCommitMessage = new TXCommitMessage();
+
+    ByteArrayData testStream = new ByteArrayData();
+    assertTrue(testStream.isEmpty());
+
+    DataOutputStream out = testStream.getDataOutput();
+    findRemoteTXMessageReply.toData(out);
+    assertTrue(testStream.size() > 0);
+
+    DataInput in = testStream.getDataInput();
+    FindRemoteTXMessage.FindRemoteTXMessageReply findRemoteTXMessageReplyFromData =
+        new FindRemoteTXMessage.FindRemoteTXMessageReply();
+    findRemoteTXMessageReplyFromData.fromData(in);
+
+    assertEquals(true, findRemoteTXMessageReplyFromData.isHostingTx);
+    assertNull(findRemoteTXMessageReplyFromData.txCommitMessage);
+  }
+}
diff --git a/geode-core/src/test/resources/org/apache/geode/codeAnalysis/sanctionedDataSerializables.txt b/geode-core/src/test/resources/org/apache/geode/codeAnalysis/sanctionedDataSerializables.txt
index e9a08a5..cf2c87a 100644
--- a/geode-core/src/test/resources/org/apache/geode/codeAnalysis/sanctionedDataSerializables.txt
+++ b/geode-core/src/test/resources/org/apache/geode/codeAnalysis/sanctionedDataSerializables.txt
@@ -1031,8 +1031,8 @@ fromData,27,2a2bb700452a2bb80046c00047b500032a2bb900480100b50004b1
 toData,24,2a2bb700422ab400032bb800432b2ab40004b900440200b1
 
 org/apache/geode/internal/cache/FindRemoteTXMessage$FindRemoteTXMessageReply,2
-fromData,46,2a2bb7000a2a2bb9000b0100b500042bb9000b01009900182a2bb9000b0100b500072a2bb8000cc0000db50006b1
-toData,66,2a2bb700032b2ab40004b9000502002ab40006c6000704a70004033d2b1cb9000502001c99001d2b2ab40007b9000502002ab4000601b600082ab400062bb80009b1
+fromData,46,2a2bb700062a2bb900070100b500042bb9000701009900182a2bb900070100b500082a2bb80009c0000ab5000bb1
+toData,25,2a2bb700032b2ab40004b900050200033d2b1cb900050200b1
 
 org/apache/geode/internal/cache/FindVersionTagOperation$FindVersionTagMessage,2
 fromData,55,2a2bb700232a2bb900240100b500032a2bb900250100b500042abb002659b70027b500052ab400052bb800282a2bb900290100b50006b1

-- 
To stop receiving notification emails like this one, please contact
eshu11@apache.org.