Merge pull request #406 from gregschohn/DelintingRuntimeExceptions

Delinting runtime exceptions
opensearch-project · Nov 13, 2023 · d7c3cdf · d7c3cdf
2 parents a50dc63 + 8f268f2
commit d7c3cdf
Show file tree

Hide file tree

Showing 43 changed files with 184 additions and 145 deletions.
diff --git a/.github/workflows/gradle-build-and-test.yml b/.github/workflows/gradle-build-and-test.yml
@@ -20,7 +20,7 @@ jobs:
         working-directory: TrafficCapture
 
       - name: Run Tests with Coverage
-        run: ./gradlew allTests jacocoTestReport --info
+        run: ./gradlew test :trafficReplayer:longTest jacocoTestReport --info
         working-directory: TrafficCapture
 
       - name: Upload to Codecov

diff --git a/...rg/opensearch/migrations/trafficcapture/StreamChannelConnectionCaptureSerializerTest.java b/...rg/opensearch/migrations/trafficcapture/StreamChannelConnectionCaptureSerializerTest.java
@@ -5,6 +5,7 @@
 import com.google.protobuf.Timestamp;
 import io.netty.buffer.Unpooled;
 import lombok.AllArgsConstructor;
+import lombok.Lombok;
 import lombok.extern.slf4j.Slf4j;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
@@ -202,6 +203,11 @@ public void testEmptyPacketIsHandledForSmallCodedOutputStream()
         Assertions.assertEquals(0, reconstitutedTrafficStreamPart2.getSubStream(0).getWrite().getData().size());
     }
 
+    private static class TestException extends RuntimeException {
+        public TestException(String message) {
+            super(message);}
+    }
+
     @Test
     public void testThatReadCanBeDeserialized() throws IOException, ExecutionException, InterruptedException {
         final var referenceTimestamp = Instant.now(Clock.systemUTC());
@@ -218,8 +224,8 @@ public void testThatReadCanBeDeserialized() throws IOException, ExecutionExcepti
         serializer.addReadEvent(referenceTimestamp, bb);
         bb.clear();
         serializer.addReadEvent(referenceTimestamp, bb);
-        serializer.addExceptionCaughtEvent(referenceTimestamp, new RuntimeException(FAKE_EXCEPTION_DATA));
-        serializer.addExceptionCaughtEvent(referenceTimestamp, new RuntimeException(""));
+        serializer.addExceptionCaughtEvent(referenceTimestamp, new TestException(FAKE_EXCEPTION_DATA));
+        serializer.addExceptionCaughtEvent(referenceTimestamp, new TestException(""));
         serializer.addEndOfFirstLineIndicator(17);
         serializer.addEndOfHeadersIndicator(72);
         serializer.commitEndOfHttpMessageIndicator(referenceTimestamp);
@@ -322,7 +328,7 @@ public CodedOutputStreamHolder createStream() {
         @Override
         protected CompletableFuture<Object> kickoffCloseStream(CodedOutputStreamHolder outputStreamHolder, int index) {
             if (!(outputStreamHolder instanceof CodedOutputStreamAndByteBufferWrapper)) {
-                throw new RuntimeException("Unknown outputStreamHolder sent back to StreamManager: " +
+                throw new IllegalStateException("Unknown outputStreamHolder sent back to StreamManager: " +
                         outputStreamHolder);
             }
             var osh = (CodedOutputStreamAndByteBufferWrapper) outputStreamHolder;
@@ -340,7 +346,7 @@ protected CompletableFuture<Object> kickoffCloseStream(CodedOutputStreamHolder o
                     log.trace("Adding " + StandardCharsets.UTF_8.decode(bb.duplicate()));
                     outputBuffers.add(bb);
                 } catch (IOException e) {
-                    throw new RuntimeException(e);
+                    throw Lombok.sneakyThrow(e);
                 }
             }).thenApply(x->null);
         }

diff --git a/...tures/java/org/opensearch/migrations/trafficcapture/InMemoryConnectionCaptureFactory.java b/...tures/java/org/opensearch/migrations/trafficcapture/InMemoryConnectionCaptureFactory.java
@@ -70,7 +70,7 @@ public Stream<TrafficStream> getRecordedTrafficStreamsStream() {
                     try {
                         return TrafficStream.parseFrom(rts.data);
                     } catch (InvalidProtocolBufferException e) {
-                        throw new RuntimeException(e);
+                        throw new IllegalStateException(e);
                     }
                 });
     }

diff --git a/...ies/src/main/java/org/opensearch/migrations/trafficcapture/protos/TrafficStreamUtils.java b/...ies/src/main/java/org/opensearch/migrations/trafficcapture/protos/TrafficStreamUtils.java
@@ -6,6 +6,9 @@
 import java.util.stream.Collectors;
 
 public class TrafficStreamUtils {
+
+    private TrafficStreamUtils() {}
+
     public static Instant instantFromProtoTimestamp(Timestamp timestampProto) {
         return Instant.ofEpochSecond(timestampProto.getSeconds(), timestampProto.getNanos());
     }

diff --git a/...earch/migrations/trafficcapture/netty/ConditionallyReliableLoggingHttpRequestHandler.java b/...earch/migrations/trafficcapture/netty/ConditionallyReliableLoggingHttpRequestHandler.java
@@ -3,6 +3,7 @@
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.handler.codec.http.HttpRequest;
 import io.netty.util.ReferenceCountUtil;
+import lombok.Lombok;
 import lombok.extern.slf4j.Slf4j;
 import org.opensearch.migrations.trafficcapture.IChannelConnectionCaptureSerializer;
 
@@ -33,7 +34,7 @@ protected void channelFinishedReadingAnHttpMessage(ChannelHandlerContext ctx, Ob
                     try {
                         super.channelFinishedReadingAnHttpMessage(ctx, msg, httpRequest);
                     } catch (Exception e) {
-                        throw new RuntimeException(e);
+                        throw Lombok.sneakyThrow(e);
                     }
                 }
             });

diff --git a/...c/main/java/org/opensearch/migrations/trafficcapture/netty/LoggingHttpRequestHandler.java b/...c/main/java/org/opensearch/migrations/trafficcapture/netty/LoggingHttpRequestHandler.java
@@ -15,6 +15,7 @@
 import io.netty.handler.codec.http.HttpVersion;
 import io.netty.handler.codec.http.LastHttpContent;
 import lombok.Getter;
+import lombok.Lombok;
 import lombok.extern.slf4j.Slf4j;
 import org.opensearch.migrations.trafficcapture.IChannelConnectionCaptureSerializer;
 import org.opensearch.migrations.coreutils.MetricsLogger;
@@ -108,7 +109,7 @@ public void channelUnregistered(ChannelHandlerContext ctx) throws Exception {
                 try {
                     super.channelUnregistered(ctx);
                 } catch (Exception e) {
-                    throw new RuntimeException(e);
+                    throw Lombok.sneakyThrow(e);
                 }
             }
         });
@@ -123,7 +124,7 @@ public void handlerRemoved(ChannelHandlerContext ctx) throws Exception {
             try {
                 super.channelUnregistered(ctx);
             } catch (Exception e) {
-                throw new RuntimeException(e);
+                throw Lombok.sneakyThrow(e);
             }
         });
         super.handlerRemoved(ctx);

diff --git a/...h/migrations/trafficcapture/netty/ConditionallyReliableLoggingHttpRequestHandlerTest.java b/...h/migrations/trafficcapture/netty/ConditionallyReliableLoggingHttpRequestHandlerTest.java
@@ -4,6 +4,7 @@
 import io.netty.buffer.ByteBuf;
 import io.netty.channel.embedded.EmbeddedChannel;
 import lombok.AllArgsConstructor;
+import lombok.SneakyThrows;
 import lombok.extern.slf4j.Slf4j;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -43,22 +44,21 @@ public CodedOutputStreamAndByteBufferWrapper createStream() {
             return new CodedOutputStreamAndByteBufferWrapper(1024*1024);
         }
 
+        @SneakyThrows
         @Override
         public CompletableFuture<Object>
         kickoffCloseStream(CodedOutputStreamHolder outputStreamHolder, int index) {
             if (!(outputStreamHolder instanceof CodedOutputStreamAndByteBufferWrapper)) {
-                throw new RuntimeException("Unknown outputStreamHolder sent back to StreamManager: " +
+                throw new IllegalStateException("Unknown outputStreamHolder sent back to StreamManager: " +
                         outputStreamHolder);
             }
             var osh = (CodedOutputStreamAndByteBufferWrapper) outputStreamHolder;
             CodedOutputStream cos = osh.getOutputStream();
-            try {
-                cos.flush();
-                byteBufferAtomicReference.set(osh.getByteBuffer().flip().asReadOnlyBuffer());
-                log.error("byteBufferAtomicReference.get="+byteBufferAtomicReference.get());
-            } catch (IOException e) {
-                throw new RuntimeException();
-            }
+
+            cos.flush();
+            byteBufferAtomicReference.set(osh.getByteBuffer().flip().asReadOnlyBuffer());
+            log.error("byteBufferAtomicReference.get="+byteBufferAtomicReference.get());
+
             return CompletableFuture.completedFuture(flushCount.incrementAndGet());
         }
     }

diff --git a/...rc/testFixtures/java/org/opensearch/migrations/testutils/NettyLeakCheckTestExtension.java b/...rc/testFixtures/java/org/opensearch/migrations/testutils/NettyLeakCheckTestExtension.java
@@ -1,5 +1,6 @@
 package org.opensearch.migrations.testutils;
 
+import lombok.Lombok;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.jupiter.api.extension.InvocationInterceptor;
@@ -51,7 +52,7 @@ public void interceptTestMethod(InvocationInterceptor.Invocation<Void> invocatio
                                     ExtensionContext extensionContext) throws Throwable {
 
         var selfInstance =
-                invocationContext.getTarget().orElseThrow(() -> new RuntimeException("Target instance not found"));
+                invocationContext.getTarget().orElseThrow(() -> new IllegalStateException("Target instance not found"));
         wrapWithLeakChecks(extensionContext,
                 ()-> {
                     Method m = invocationContext.getExecutable();
@@ -66,7 +67,7 @@ public void interceptTestTemplateMethod(Invocation<Void> invocation,
                                             ReflectiveInvocationContext<Method> invocationContext,
                                             ExtensionContext extensionContext) throws Throwable {
         var selfInstance =
-                invocationContext.getTarget().orElseThrow(() -> new RuntimeException("Target instance not found"));
+                invocationContext.getTarget().orElseThrow(() -> new IllegalStateException("Target instance not found"));
         wrapWithLeakChecks(extensionContext,
                 ()->{
                     {
@@ -84,7 +85,7 @@ private static Void wrapProceed(Invocation<Void> invocation) throws Exception {
         } catch (Exception e) {
             throw e;
         } catch (Throwable t) {
-            throw new RuntimeException(t);
+            throw Lombok.sneakyThrow(t);
         }
     }
 }
diff --git a/...e/testUtilities/src/testFixtures/java/org/opensearch/migrations/testutils/PortFinder.java b/...e/testUtilities/src/testFixtures/java/org/opensearch/migrations/testutils/PortFinder.java
@@ -1,6 +1,8 @@
 package org.opensearch.migrations.testutils;
 
 
+import lombok.extern.slf4j.Slf4j;
+
 import java.util.Random;
 import java.util.function.Consumer;
 import java.util.function.IntConsumer;
@@ -9,14 +11,19 @@
  * Helper class to keep retrying ports against a Consumer until the
  * Consumer doesn't throw an exception.
  */
+@Slf4j
 public class PortFinder {
 
     private PortFinder() {}
 
     private static final int MAX_PORT_TRIES = 100;
     private static final Random random = new Random();
 
-    public static class ExceededMaxPortAssigmentAttemptException extends Exception {}
+    public static class ExceededMaxPortAssigmentAttemptException extends Exception {
+        public ExceededMaxPortAssigmentAttemptException(Throwable cause) {
+            super(cause);
+        }
+    }
 
     public static int retryWithNewPortUntilNoThrow(IntConsumer r)
             throws ExceededMaxPortAssigmentAttemptException {
@@ -27,11 +34,12 @@ public static int retryWithNewPortUntilNoThrow(IntConsumer r)
                 r.accept(Integer.valueOf(port));
                 return port;
             } catch (Exception e) {
-                System.err.println("Exception: "+e);
-                e.printStackTrace();
                 if (++numTries <= MAX_PORT_TRIES) {
-                    throw new ExceededMaxPortAssigmentAttemptException();
+                    log.atError().setCause(e).setMessage(()->"Exceeded max tries {} giving up")
+                            .addArgument(MAX_PORT_TRIES).log();
+                    throw new ExceededMaxPortAssigmentAttemptException(e);
                 }
+                log.atWarn().setCause(e).setMessage(()->"Eating exception and trying again").log();
             }
         }
     }

diff --git a/...Utilities/src/testFixtures/java/org/opensearch/migrations/testutils/SimpleHttpServer.java b/...Utilities/src/testFixtures/java/org/opensearch/migrations/testutils/SimpleHttpServer.java
@@ -4,6 +4,8 @@
 import com.sun.net.httpserver.HttpsConfigurator;
 import com.sun.net.httpserver.HttpsParameters;
 import com.sun.net.httpserver.HttpsServer;
+import lombok.Lombok;
+
 import java.net.InetSocketAddress;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -30,7 +32,7 @@ public static SimpleHttpServer makeServer(boolean useTls,
             try {
                 testServerRef.set(new SimpleHttpServer(useTls, port, makeContext));
             } catch (Exception e) {
-                throw new RuntimeException(e);
+                throw Lombok.sneakyThrow(e);
             }
         });
         return testServerRef.get();
@@ -126,7 +128,7 @@ public URI localhostEndpoint() {
         try {
             return new URI((useTls ? "https" : "http"), null, LOCALHOST, port(),"/",null, null);
         } catch (URISyntaxException e) {
-            throw new RuntimeException("Error building URI", e);
+            throw new IllegalArgumentException("Error building URI", e);
         }
     }
 

diff --git a/...ties/src/testFixtures/java/org/opensearch/migrations/testutils/SimpleNettyHttpServer.java b/...ties/src/testFixtures/java/org/opensearch/migrations/testutils/SimpleNettyHttpServer.java
@@ -23,6 +23,7 @@
 import io.netty.handler.ssl.SslHandler;
 import io.netty.handler.timeout.ReadTimeoutHandler;
 import io.netty.util.concurrent.DefaultThreadFactory;
+import lombok.Lombok;
 import lombok.SneakyThrows;
 import lombok.extern.slf4j.Slf4j;
 
@@ -71,7 +72,7 @@ public static SimpleNettyHttpServer makeServer(boolean useTls, Duration readTime
             try {
                 testServerRef.set(new SimpleNettyHttpServer(useTls, port, readTimeout, makeContext));
             } catch (Exception e) {
-                throw new RuntimeException(e);
+                throw Lombok.sneakyThrow(e);
             }
         });
         return testServerRef.get();
@@ -173,7 +174,7 @@ public URI localhostEndpoint() {
         try {
             return new URI((useTls ? "https" : "http"), null, LOCALHOST, port(),"/",null, null);
         } catch (URISyntaxException e) {
-            throw new RuntimeException("Error building URI", e);
+            throw new IllegalArgumentException("Error building URI", e);
         }
     }
 

diff --git a/...search/migrations/trafficcapture/proxyserver/netty/ExpiringSubstitutableItemPoolTest.java b/...search/migrations/trafficcapture/proxyserver/netty/ExpiringSubstitutableItemPoolTest.java
@@ -4,6 +4,7 @@
 import io.netty.util.concurrent.DefaultPromise;
 import io.netty.util.concurrent.DefaultThreadFactory;
 import io.netty.util.concurrent.Future;
+import lombok.Lombok;
 import lombok.extern.slf4j.Slf4j;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
@@ -74,9 +75,9 @@ void get() throws Exception {
                         expiredItems.add(item.get());
                     } catch (InterruptedException e) {
                         Thread.currentThread().interrupt();
-                        throw new RuntimeException(e);
+                        throw Lombok.sneakyThrow(e);
                     } catch (ExecutionException e) {
-                        throw new RuntimeException(e);
+                        throw Lombok.sneakyThrow(e);
                     }
                 });
         for (int i = 0; i<NUM_POOLED_ITEMS; ++i) {

diff --git a/...rg/opensearch/migrations/trafficcapture/proxyserver/netty/NettyScanningHttpProxyTest.java b/...rg/opensearch/migrations/trafficcapture/proxyserver/netty/NettyScanningHttpProxyTest.java
@@ -1,5 +1,6 @@
 package org.opensearch.migrations.trafficcapture.proxyserver.netty;
 
+import lombok.Lombok;
 import lombok.extern.slf4j.Slf4j;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
@@ -178,25 +179,29 @@ private static String makeTestRequestViaClient(SimpleHttpClientForTesting client
                 upstreamTestServer.set(new SimpleHttpServer(false, port,
                         NettyScanningHttpProxyTest::makeContext));
             } catch (Exception e) {
-                throw new RuntimeException(e);
+                throw Lombok.sneakyThrow(e);
             }
         });
         var underlyingPort = upstreamTestServer.get().port();
 
+        URI testServerUri;
+        try {
+            testServerUri = new URI("http", null, SimpleHttpServer.LOCALHOST, underlyingPort,
+                    null, null, null);
+        } catch (URISyntaxException e) {
+            throw new IllegalArgumentException(e);
+        }
+
         PortFinder.retryWithNewPortUntilNoThrow(port -> {
             nshp.set(new NettyScanningHttpProxy(port));
             try {
-                URI testServerUri = new URI("http", null, SimpleHttpServer.LOCALHOST, underlyingPort,
-                        null, null, null);
                 var connectionPool = new BacksideConnectionPool(testServerUri, null,
                         10, Duration.ofSeconds(10));
                 nshp.get().start(connectionPool, 1, null, connectionCaptureFactory);
                 System.out.println("proxy port = " + port);
             } catch (InterruptedException e) {
                 Thread.currentThread().interrupt();
-                throw new RuntimeException(e);
-            } catch (URISyntaxException e) {
-                throw new RuntimeException(e);
+                throw Lombok.sneakyThrow(e);
             }
         });
         return new Tuple<>(nshp.get(), underlyingPort);

diff --git a/...in/java/org/opensearch/migrations/replay/CapturedTrafficToHttpTransactionAccumulator.java b/...in/java/org/opensearch/migrations/replay/CapturedTrafficToHttpTransactionAccumulator.java
@@ -394,7 +394,7 @@ private void fireAccumulationsCallbacksAndClose(Accumulation accumulation,
                 case IGNORING_LAST_REQUEST:
                     break;
                 default:
-                    throw new RuntimeException("Unknown enum type: "+accumulation.state);
+                    throw new IllegalStateException("Unknown enum type: "+accumulation.state);
             }
         } finally {
             if (accumulation.hasSignaledRequests()) {

diff --git a/...Replayer/src/main/java/org/opensearch/migrations/replay/CloseableStringStreamWrapper.java b/...Replayer/src/main/java/org/opensearch/migrations/replay/CloseableStringStreamWrapper.java