This is an automated email from the ASF dual-hosted git repository. bschuchardt pushed a commit to branch feature/GEODE-8020revert in repository https://gitbox.apache.org/repos/asf/geode.git
commit 22cc6b5c8200069d2282ca296af15f8025af2821 Author: Bruce Schuchardt <[email protected]> AuthorDate: Thu Apr 30 09:51:45 2020 -0700 Revert "GEODE-8020: buffer corruption in SSL communications (#4994)" This reverts commit ec8db54ad7f342542762beb8f3e912dff44e3a53. The fix for GEODE-8020 reduced performance in SSL benchmark tests. Unfortunately this revert leaves SSL communications broken, but there are no CI tests that show that this is the case. --- .../geode/ClusterCommunicationsDUnitTest.java | 33 +--------------------- .../apache/geode/internal/net/NioSslEngine.java | 4 +-- .../org/apache/geode/internal/tcp/Connection.java | 16 ++++------- .../geode/internal/net/NioSslEngineTest.java | 5 ---- 4 files changed, 9 insertions(+), 49 deletions(-) diff --git a/geode-core/src/distributedTest/java/org/apache/geode/ClusterCommunicationsDUnitTest.java b/geode-core/src/distributedTest/java/org/apache/geode/ClusterCommunicationsDUnitTest.java index 09f94e5..3ae79fd 100644 --- a/geode-core/src/distributedTest/java/org/apache/geode/ClusterCommunicationsDUnitTest.java +++ b/geode-core/src/distributedTest/java/org/apache/geode/ClusterCommunicationsDUnitTest.java @@ -43,8 +43,6 @@ import java.io.DataOutput; import java.io.File; import java.io.IOException; import java.io.Serializable; -import java.lang.reflect.Field; -import java.nio.Buffer; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -71,7 +69,6 @@ import org.apache.geode.distributed.Locator; import org.apache.geode.distributed.internal.ClusterDistributionManager; import org.apache.geode.distributed.internal.DMStats; import org.apache.geode.distributed.internal.DirectReplyProcessor; -import org.apache.geode.distributed.internal.DistributionImpl; import org.apache.geode.distributed.internal.DistributionMessage; import org.apache.geode.distributed.internal.InternalDistributedSystem; import org.apache.geode.distributed.internal.MessageWithReply; @@ -79,15 +76,12 @@ import org.apache.geode.distributed.internal.OperationExecutors; import org.apache.geode.distributed.internal.ReplyException; import org.apache.geode.distributed.internal.ReplyMessage; import org.apache.geode.distributed.internal.SerialAckedMessage; -import org.apache.geode.distributed.internal.membership.InternalDistributedMember; import org.apache.geode.distributed.internal.membership.gms.membership.GMSJoinLeave; import org.apache.geode.internal.AvailablePortHelper; import org.apache.geode.internal.InternalDataSerializer; import org.apache.geode.internal.cache.DirectReplyMessage; -import org.apache.geode.internal.cache.InternalCache; import org.apache.geode.internal.serialization.DeserializationContext; import org.apache.geode.internal.serialization.SerializationContext; -import org.apache.geode.internal.tcp.Connection; import org.apache.geode.test.dunit.DistributedTestUtils; import org.apache.geode.test.dunit.Host; import org.apache.geode.test.dunit.VM; @@ -149,7 +143,7 @@ public class ClusterCommunicationsDUnitTest implements Serializable { } @Test - public void createEntryAndVerifyUpdate() throws Exception { + public void createEntryAndVerifyUpdate() { int locatorPort = createLocator(getVM(0)); for (int i = 1; i <= NUM_SERVERS; i++) { createCacheAndRegion(getVM(i), locatorPort); @@ -163,9 +157,6 @@ public class ClusterCommunicationsDUnitTest implements Serializable { } for (int i = 1; i <= NUM_SERVERS; i++) { verifyUpdatedEntry(getVM(i)); - if (!disableTcp) { - verifyBufferType(getVM(i)); - } } } @@ -254,28 +245,6 @@ public class ClusterCommunicationsDUnitTest implements Serializable { }); } - private void verifyBufferType(VM vm) throws Exception { - vm.invoke("verify connection type", () -> { - assertThat(cache).isNotNull(); - InternalCache internalCache = (InternalCache) cache; - final DistributionImpl distribution = - (DistributionImpl) internalCache.getDistributionManager().getDistribution(); - InternalDistributedMember locatorMember = - (InternalDistributedMember) distribution.getCoordinator(); - final Connection connection = - distribution.getDirectChannel().getConduit().getConnection(locatorMember, false, false, - System.currentTimeMillis(), 15000, 0); - Field inputBufferField = Connection.class.getDeclaredField("inputBuffer"); - inputBufferField.setAccessible(true); - Buffer inputBuffer = (Buffer) inputBufferField.get(connection); - // SSL connections use heap buffers for decoding efficiency. Non-SSL connections use - // direct buffers since they don't need to be accessed as much - assertThat(inputBuffer.isDirect()).isNotEqualTo(useSSL); - }); - } - - - private void performCreate(VM memberVM) { memberVM.invoke("perform create", () -> cache .getRegion(regionName).put("testKey", "testValue")); diff --git a/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java b/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java index 424e53a..2d55fa3 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java +++ b/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java @@ -74,7 +74,7 @@ public class NioSslEngine implements NioFilter { int packetBufferSize = engine.getSession().getPacketBufferSize(); this.engine = engine; this.bufferPool = bufferPool; - this.myNetData = bufferPool.acquireNonDirectSenderBuffer(packetBufferSize); + this.myNetData = bufferPool.acquireDirectSenderBuffer(packetBufferSize); this.peerAppData = bufferPool.acquireNonDirectReceiveBuffer(appBufferSize); } @@ -301,7 +301,7 @@ public class NioSslEngine implements NioFilter { ByteBuffer buffer = wrappedBuffer; int requiredSize = engine.getSession().getPacketBufferSize(); if (buffer == null) { - buffer = bufferPool.acquireNonDirectBuffer(bufferType, requiredSize); + buffer = bufferPool.acquireDirectBuffer(bufferType, requiredSize); } else if (buffer.capacity() < requiredSize) { buffer = bufferPool.expandWriteBufferIfNeeded(bufferType, buffer, requiredSize); } diff --git a/geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java b/geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java index 089eb2e..d384aa1 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java +++ b/geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java @@ -1662,8 +1662,8 @@ public class Connection implements Runnable { } catch (IOException e) { // "Socket closed" check needed for Solaris jdk 1.4.2_08 if (!isSocketClosed() && !"Socket closed".equalsIgnoreCase(e.getMessage())) { - if (logger.isInfoEnabled() && !isIgnorableIOException(e)) { - logger.info("{} io exception for {}", p2pReaderName(), this, e); + if (logger.isDebugEnabled() && !isIgnorableIOException(e)) { + logger.debug("{} io exception for {}", p2pReaderName(), this, e); } if (e.getMessage().contains("interrupted by a call to WSACancelBlockingCall")) { if (logger.isDebugEnabled()) { @@ -1720,7 +1720,7 @@ public class Connection implements Runnable { if (inputBuffer != null) { getBufferPool().releaseReceiveBuffer(inputBuffer); } - inputBuffer = getBufferPool().acquireNonDirectReceiveBuffer(packetBufferSize); + inputBuffer = getBufferPool().acquireDirectReceiveBuffer(packetBufferSize); } if (channel.socket().getReceiveBufferSize() < packetBufferSize) { channel.socket().setReceiveBufferSize(packetBufferSize); @@ -1763,13 +1763,9 @@ public class Connection implements Runnable { } msg = msg.toLowerCase(); - - if (e instanceof SSLException && msg.contains("status = closed")) { - return true; // engine has been closed - this is normal - } - - return (msg.contains("forcibly closed") || msg.contains("reset by peer") - || msg.contains("connection reset") || msg.contains("socket is closed")); + return msg.contains("forcibly closed") + || msg.contains("reset by peer") + || msg.contains("connection reset"); } private static boolean validMsgType(int msgType) { diff --git a/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java b/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java index 720ef62..b42d566 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java @@ -86,11 +86,6 @@ public class NioSslEngineTest { } @Test - public void engineUsesHeapBuffers() { - assertThat(nioSslEngine.myNetData.isDirect()).isFalse(); - } - - @Test public void handshake() throws Exception { SocketChannel mockChannel = mock(SocketChannel.class); when(mockChannel.read(any(ByteBuffer.class))).thenReturn(100, 100, 100, 0);
