From 7f353cc40d3ad6cdba1f74f6152ef73dc0a750e1 Mon Sep 17 00:00:00 2001 From: stack Date: Thu, 2 Apr 2020 20:29:58 -0700 Subject: [PATCH] HBASE-24107 [Flakey Test] TestThriftServerCmdLine.testRunThriftServer NPEs if InfoServer port clash --- .../thrift/TestBindExceptionHandling.java | 21 ++++- .../hbase/thrift/TestThriftServerCmdLine.java | 80 ++++++++++++++----- 2 files changed, 78 insertions(+), 23 deletions(-) diff --git a/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestBindExceptionHandling.java b/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestBindExceptionHandling.java index fa1d141df82..9fc95420b8e 100644 --- a/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestBindExceptionHandling.java +++ b/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestBindExceptionHandling.java @@ -32,14 +32,29 @@ public class TestBindExceptionHandling { HBaseClassTestRule.forClass(TestBindExceptionHandling.class); /** - * See if random port choosing works around port clashes + * See if random port choosing works around protocol port clashes */ @Test - public void testPortClash() { + public void testProtocolPortClash() { ThriftServer thriftServer = null; try { thriftServer = new TestThriftServerCmdLine(null, false, false, false). - createBoundServer(true); + createBoundServer(true, false); + assertNotNull(thriftServer.tserver); + } finally { + thriftServer.stop(); + } + } + + /** + * See if random port choosing works around protocol port clashes + */ + @Test + public void testInfoPortClash() { + ThriftServer thriftServer = null; + try { + thriftServer = new TestThriftServerCmdLine(null, false, false, false). + createBoundServer(false, true); assertNotNull(thriftServer.tserver); } finally { thriftServer.stop(); diff --git a/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java b/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java index 1aa0a5c4d3f..2607c2fe002 100644 --- a/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java +++ b/hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java @@ -49,7 +49,6 @@ import org.apache.thrift.server.TServer; import org.apache.thrift.transport.TFramedTransport; import org.apache.thrift.transport.TSocket; import org.apache.thrift.transport.TTransport; -import org.apache.thrift.transport.TTransportException; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.ClassRule; @@ -175,16 +174,40 @@ public class TestThriftServerCmdLine { * Server can fail to bind if clashing address. Add retrying until we get a good server. */ ThriftServer createBoundServer() { - return createBoundServer(false); + return createBoundServer(false, false); + } + + private ServerSocket getBoundSocket() { + ServerSocket ss = null; + while (true) { + port = getRandomPort(); + try { + ss = new ServerSocket(); + ss.bind(new InetSocketAddress(port)); + break; + } catch (IOException ioe) { + LOG.warn("Failed bind", ioe); + try { + ss.close(); + } catch (IOException ioe2) { + LOG.warn("FAILED CLOSE of failed bind socket", ioe2); + } + } + } + return ss; } /** - * @param clash This param is just so we can manufacture a port clash so we can test the - * code does the right thing when this happens during actual test runs. Ugly but works. - * @see TestBindExceptionHandling#testPortClash() + * @param protocolPortClash This param is just so we can manufacture a port clash so we can test + * the code does the right thing when this happens during actual test runs. Ugly but works. + * @see TestBindExceptionHandling#testProtocolPortClash() */ - ThriftServer createBoundServer(boolean clash) { - boolean testClashOfFirstPort = clash; + ThriftServer createBoundServer(boolean protocolPortClash, boolean infoPortClash) { + if (protocolPortClash && infoPortClash) { + throw new RuntimeException("Can't set both at same time"); + } + boolean testClashOfFirstProtocolPort = protocolPortClash; + boolean testClashOfFirstInfoPort = infoPortClash; List args = new ArrayList<>(); ServerSocket ss = null; for (int i = 0; i < 100; i++) { @@ -194,22 +217,26 @@ public class TestThriftServerCmdLine { assertTrue(serverTypeOption.startsWith("-")); args.add(serverTypeOption); } - port = getRandomPort(); - if (testClashOfFirstPort) { + if (testClashOfFirstProtocolPort) { // Test what happens if already something bound to the socket. // Occupy the random port we just pulled. - try { - ss = new ServerSocket(); - ss.bind(new InetSocketAddress(port)); - } catch (IOException ioe) { - LOG.warn("Failed bind", ioe); - } - testClashOfFirstPort = false; + ss = getBoundSocket(); + port = ss.getLocalPort(); + testClashOfFirstProtocolPort = false; + } else { + port = getRandomPort(); } args.add("-" + PORT_OPTION); args.add(String.valueOf(port)); args.add("-" + INFOPORT_OPTION); - int infoPort = getRandomPort(); + int infoPort; + if (testClashOfFirstInfoPort) { + ss = getBoundSocket(); + infoPort = ss.getLocalPort(); + testClashOfFirstInfoPort = false; + } else { + infoPort = getRandomPort(); + } args.add(String.valueOf(infoPort)); if (specifyFramed) { @@ -230,9 +257,8 @@ public class TestThriftServerCmdLine { for (int ii = 0; ii < 100 && (thriftServer.tserver == null); ii++) { Threads.sleep(100); } - if (cmdLineException instanceof TTransportException && - cmdLineException.getCause() instanceof BindException) { - LOG.info("Trying new port", cmdLineException); + if (isBindException(cmdLineException)) { + LOG.info("BindException; trying new port", cmdLineException); cmdLineException = null; thriftServer.stop(); continue; @@ -253,6 +279,20 @@ public class TestThriftServerCmdLine { return thriftServer; } + private boolean isBindException(Exception cmdLineException) { + if (cmdLineException == null) { + return false; + } + if (cmdLineException instanceof BindException) { + return true; + } + if (cmdLineException.getCause() != null && + cmdLineException.getCause() instanceof BindException) { + return true; + } + return false; + } + @Test public void testRunThriftServer() throws Exception { ThriftServer thriftServer = createBoundServer();