You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by bs...@apache.org on 2020/03/23 18:26:49 UTC
[geode] 01/01: GEODE-6008: CI Failure:
ClientServerHostNameVerificationDistributedTest.expectConnectionFailureWhenNoHostNameInServerKey
failed
This is an automated email from the ASF dual-hosted git repository.
bschuchardt pushed a commit to branch feature/GEODE-6008b
in repository https://gitbox.apache.org/repos/asf/geode.git
commit 97a0dc87980f1c2c42351c84ec32a2411d292251
Author: Bruce Schuchardt <bs...@pivotal.io>
AuthorDate: Mon Mar 23 11:09:53 2020 -0700
GEODE-6008: CI Failure: ClientServerHostNameVerificationDistributedTest.expectConnectionFailureWhenNoHostNameInServerKey failed
The "Message distribution has terminated" failure is caused by an
unreported NullPointerException in a residual reader thread introduced
in the fix for GEODE-7727. That fix caused a thread to say alive in a
peer-to-peer tcp/ip Connection in order to clean up the receiving side
of a socket. The Connection shutdown method close() method, however,
releases the Connection's input buffer and nulls out the field. The
reader thread then threw an NPE that was caught and caused the
"Message distribution has terminated" message, which is picked up as a
suspect string by the testing infrastructure.
This problem is also seen in GEODE-7894, GEODE-7871, GEODE-7873 and
GEODE-7806.
The fix is to record the fact that a residual reader thread exists and
avoid releasing the Connection's input buffer when the connection is
closed. This lets the reader thread do the cleanup.
While testing the fix I found that the NioSslEngine was throwing an
IllegalStateException when the reader thread tried to use it in this
same situation. This exception wasn't being caught and caused more
suspect strings to be logged. I've changed this to a checked exception
that is already handled by the reader thread.
ClientServerHostNameVerificationDistributedTest also wasn't working on
my Mac due to its /etc/hosts configuration. I changed the test to allow
the IP address selected by LocalHostUtil to be a valid client/server
address for the SSL certificates it generates.
---
...tServerHostNameVerificationDistributedTest.java | 4 +++
.../geode/internal/tcp/CloseConnectionTest.java | 13 ++++++++
.../apache/geode/internal/net/NioSslEngine.java | 4 +--
.../org/apache/geode/internal/tcp/Connection.java | 38 ++++++++++++++++++----
.../geode/internal/net/NioSslEngineTest.java | 8 +++--
5 files changed, 55 insertions(+), 12 deletions(-)
diff --git a/geode-core/src/distributedTest/java/org/apache/geode/cache/client/internal/ClientServerHostNameVerificationDistributedTest.java b/geode-core/src/distributedTest/java/org/apache/geode/cache/client/internal/ClientServerHostNameVerificationDistributedTest.java
index 8f1db5f..3ef5ac0 100644
--- a/geode-core/src/distributedTest/java/org/apache/geode/cache/client/internal/ClientServerHostNameVerificationDistributedTest.java
+++ b/geode-core/src/distributedTest/java/org/apache/geode/cache/client/internal/ClientServerHostNameVerificationDistributedTest.java
@@ -39,6 +39,7 @@ import org.apache.geode.cache.client.NoAvailableServersException;
import org.apache.geode.cache.ssl.CertStores;
import org.apache.geode.cache.ssl.CertificateBuilder;
import org.apache.geode.cache.ssl.CertificateMaterial;
+import org.apache.geode.internal.inet.LocalHostUtil;
import org.apache.geode.internal.net.SocketCreatorFactory;
import org.apache.geode.test.dunit.IgnoredException;
import org.apache.geode.test.dunit.rules.ClusterStartupRule;
@@ -82,6 +83,7 @@ public class ClientServerHostNameVerificationDistributedTest {
.sanDnsName(InetAddress.getLoopbackAddress().getHostName())
.sanDnsName(InetAddress.getLocalHost().getHostName())
.sanDnsName(InetAddress.getLocalHost().getCanonicalHostName())
+ .sanIpAddress(LocalHostUtil.getLocalHost())
.sanIpAddress(InetAddress.getLocalHost())
.sanIpAddress(InetAddress.getByName("0.0.0.0")) // to pass on windows
.generate();
@@ -91,6 +93,7 @@ public class ClientServerHostNameVerificationDistributedTest {
.issuedBy(ca)
.sanDnsName(InetAddress.getLocalHost().getHostName())
.sanDnsName(InetAddress.getLocalHost().getCanonicalHostName())
+ .sanIpAddress(LocalHostUtil.getLocalHost())
.sanIpAddress(InetAddress.getLocalHost())
.generate();
@@ -162,6 +165,7 @@ public class ClientServerHostNameVerificationDistributedTest {
.sanDnsName(InetAddress.getLoopbackAddress().getHostName())
.sanDnsName(InetAddress.getLocalHost().getHostName())
.sanDnsName(InetAddress.getLocalHost().getCanonicalHostName())
+ .sanIpAddress(LocalHostUtil.getLocalHost())
.sanIpAddress(InetAddress.getLocalHost())
.generate();
diff --git a/geode-core/src/distributedTest/java/org/apache/geode/internal/tcp/CloseConnectionTest.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/tcp/CloseConnectionTest.java
index 154e908..abe8ea9 100644
--- a/geode-core/src/distributedTest/java/org/apache/geode/internal/tcp/CloseConnectionTest.java
+++ b/geode-core/src/distributedTest/java/org/apache/geode/internal/tcp/CloseConnectionTest.java
@@ -22,6 +22,8 @@ import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.distributed.internal.ClusterDistributionManager;
import org.apache.geode.distributed.internal.DistributionImpl;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
import org.apache.geode.test.dunit.VM;
import org.apache.geode.test.dunit.cache.CacheTestCase;
@@ -63,6 +65,17 @@ public class CloseConnectionTest extends CacheTestCase {
ConnectionTable conTable = getConnectionTable();
assertThat(conTable.getNumberOfReceivers()).isEqualTo(2);
});
+
+ // Make sure the Sender connection has a reader thread
+ vm1.invoke(() -> {
+ ConnectionTable conTable = getConnectionTable();
+ InternalDistributedSystem distributedSystem = getSystem();
+ InternalDistributedMember otherMember = distributedSystem.getDistributionManager()
+ .getOtherNormalDistributionManagerIds().iterator().next();
+ Connection connection = conTable.getConduit().getConnection(otherMember, true, false,
+ System.currentTimeMillis(), 15000, 0);
+ assertThat(connection.hasResidualReaderThread()).isTrue();
+ });
}
private ConnectionTable getConnectionTable() {
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 9d67594..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
@@ -212,9 +212,9 @@ public class NioSslEngine implements NioFilter {
return bufferPool.expandWriteBufferIfNeeded(type, existing, desiredCapacity);
}
- void checkClosed() {
+ void checkClosed() throws IOException {
if (closed) {
- throw new IllegalStateException("NioSslEngine has been closed");
+ throw new IOException("NioSslEngine has been closed");
}
}
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 8c8a2fc..e568f5a 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
@@ -471,6 +471,12 @@ public class Connection implements Runnable {
private volatile boolean ackTimedOut;
/**
+ * a Reader thread for an shared Connection will remain around in order to
+ * ensure that the socket is properly closed.
+ */
+ private volatile boolean hasResidualReaderThread;
+
+ /**
* creates a "reader" connection that we accepted (it was initiated by an explicit connect being
* done on the other side).
*/
@@ -1326,7 +1332,7 @@ public class Connection implements Runnable {
}
// make sure our socket is closed
asyncClose(false);
- if (!isReceiver) {
+ if (!isReceiver && !hasResidualReaderThread()) {
// receivers release the input buffer when exiting run(). Senders use the
// inputBuffer for reading direct-reply responses
releaseInputBuffer();
@@ -1465,6 +1471,7 @@ public class Connection implements Runnable {
asyncClose(false);
}
}
+
releaseInputBuffer();
// make sure that if the reader thread exits we notify a thread waiting for the handshake.
@@ -1508,6 +1515,9 @@ public class Connection implements Runnable {
}
private void readMessages() {
+ if (closing.get()) {
+ return;
+ }
// take a snapshot of uniqueId to detect reconnect attempts
SocketChannel channel;
try {
@@ -1552,7 +1562,7 @@ public class Connection implements Runnable {
}
// we should not change the state of the connection if we are a handshake reader thread
// as there is a race between this thread and the application thread doing direct ack
- boolean isHandShakeReader = false;
+ boolean handshakeHasBeenRead = false;
// if we're using SSL/TLS the input buffer may already have data to process
boolean skipInitialRead = getInputBuffer().position() > 0;
try {
@@ -1607,7 +1617,7 @@ public class Connection implements Runnable {
}
processInputBuffer();
- if (!isHandShakeReader && !isReceiver && (handshakeRead || handshakeCancelled)) {
+ if (!handshakeHasBeenRead && !isReceiver && (handshakeRead || handshakeCancelled)) {
if (logger.isDebugEnabled()) {
if (handshakeRead) {
logger.debug("handshake has been read {}", this);
@@ -1615,12 +1625,16 @@ public class Connection implements Runnable {
logger.debug("handshake has been cancelled {}", this);
}
}
- isHandShakeReader = true;
+ handshakeHasBeenRead = true;
// Once we have read the handshake for unshared connections, the reader can skip
// processing messages
if (!sharedResource || asyncMode) {
break;
+ } else {
+ // not exiting and not a Reader spawned from a ServerSocket.accept(), so
+ // let's set some state noting that this is happening
+ hasResidualReaderThread = true;
}
}
@@ -1663,7 +1677,7 @@ public class Connection implements Runnable {
return;
} catch (Exception e) {
- owner.getConduit().getCancelCriterion().checkCancelInProgress(null);
+ owner.getConduit().getCancelCriterion().checkCancelInProgress(e);
if (!stopped && !isSocketClosed()) {
logger.fatal(String.format("%s exception in channel read", p2pReaderName()), e);
}
@@ -1676,14 +1690,15 @@ public class Connection implements Runnable {
}
}
} finally {
- if (!isHandShakeReader || (sharedResource && !asyncMode)) {
+ hasResidualReaderThread = false;
+ if (!handshakeHasBeenRead || (sharedResource && !asyncMode)) {
synchronized (stateLock) {
connectionState = STATE_IDLE;
}
}
if (logger.isDebugEnabled()) {
logger.debug("readMessages terminated id={} from {} isHandshakeReader={}", conduitIdStr,
- remoteAddr, isHandShakeReader);
+ remoteAddr, handshakeHasBeenRead);
}
}
}
@@ -3236,6 +3251,15 @@ public class Connection implements Runnable {
}
/**
+ * A shared sender connection will leave a reader thread around to ensure that the
+ * socket is properly closed at this end. When that is the case isResidualReaderThread
+ * will return true.
+ */
+ public boolean hasResidualReaderThread() {
+ return hasResidualReaderThread;
+ }
+
+ /**
* answers whether this connection is used for ordered message delivery
*/
boolean getPreserveOrder() {
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 1d5e4a6..ef16a21 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
@@ -34,6 +34,7 @@ import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import java.io.IOException;
import java.net.Socket;
import java.net.SocketException;
import java.nio.ByteBuffer;
@@ -196,11 +197,11 @@ public class NioSslEngineTest {
@Test
- public void checkClosed() {
+ public void checkClosed() throws Exception {
nioSslEngine.checkClosed();
}
- @Test(expected = IllegalStateException.class)
+ @Test(expected = IOException.class)
public void checkClosedThrows() throws Exception {
when(mockEngine.wrap(any(ByteBuffer.class), any(ByteBuffer.class))).thenReturn(
new SSLEngineResult(CLOSED, FINISHED, 0, 100));
@@ -340,7 +341,8 @@ public class NioSslEngineTest {
when(mockEngine.wrap(any(ByteBuffer.class), any(ByteBuffer.class))).thenReturn(
new SSLEngineResult(CLOSED, FINISHED, 0, 0));
nioSslEngine.close(mockChannel);
- assertThatThrownBy(() -> nioSslEngine.checkClosed()).isInstanceOf(IllegalStateException.class);
+ assertThatThrownBy(() -> nioSslEngine.checkClosed()).isInstanceOf(IOException.class)
+ .hasMessageContaining("NioSslEngine has been closed");
nioSslEngine.close(mockChannel);
}