Use a single SocketHttpClient. (#4794)

* Use a single SocketHttpClient. * Refactor all usages in reactive and Níma. * Fix intermittent test failure.
helidon-io · Aug 26, 2022 · 73db971 · 73db971
1 parent ba0d5c6
commit 73db971
Show file tree

Hide file tree

Showing 56 changed files with 1,064 additions and 1,329 deletions.
diff --git a/common/testing/http-junit5/etc/spotbugs/exclude.xml b/common/testing/http-junit5/etc/spotbugs/exclude.xml
@@ -0,0 +1,31 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+    Copyright (c) 2022 Oracle and/or its affiliates.
+
+    Licensed 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.
+
+-->
+
+<FindBugsFilter
+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xmlns="https://github.com/spotbugs/filter/3.0.0"
+        xsi:schemaLocation="https://github.com/spotbugs/filter/3.0.0 https://raw.githubusercontent.com/spotbugs/spotbugs/3.1.0/spotbugs/etc/findbugsfilter.xsd">
+
+    <Match>
+        <!-- This is a testing library, we want to use plain socket -->
+        <Class name="io.helidon.common.testing.http.junit5.SocketHttpClient"/>
+        <Bug pattern="UNENCRYPTED_SOCKET"/>
+    </Match>
+
+</FindBugsFilter>
diff --git a/common/testing/http-junit5/pom.xml b/common/testing/http-junit5/pom.xml
@@ -29,6 +29,10 @@
     <artifactId>helidon-common-testing-http-junit5</artifactId>
     <name>Helidon Common Testing HTTP JUnit5</name>
 
+    <properties>
+        <spotbugs.exclude>etc/spotbugs/exclude.xml</spotbugs.exclude>
+    </properties>
+
     <dependencies>
         <dependency>
             <groupId>io.helidon.common</groupId>

diff --git a/...ommon/testing/http/HttpHeaderMatcher.java → ...esting/http/junit5/HttpHeaderMatcher.java b/...ommon/testing/http/HttpHeaderMatcher.java → ...esting/http/junit5/HttpHeaderMatcher.java
@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package io.helidon.common.testing.http;
+package io.helidon.common.testing.http.junit5;
 
 import java.util.List;
 

diff --git a/...ng/junit5/webserver/SocketHttpClient.java → ...testing/http/junit5/SocketHttpClient.java b/...ng/junit5/webserver/SocketHttpClient.java → ...testing/http/junit5/SocketHttpClient.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 Oracle and/or its affiliates.
+ * Copyright (c) 2017, 2022 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-package io.helidon.nima.testing.junit5.webserver;
+package io.helidon.common.testing.http.junit5;
 
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -26,99 +26,66 @@
 import java.net.SocketException;
 import java.nio.charset.StandardCharsets;
 import java.time.Duration;
-import java.time.temporal.ChronoUnit;
 import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
-import java.util.Map;
-import java.util.TreeMap;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import io.helidon.common.http.HeadersClientResponse;
+import io.helidon.common.http.HeadersWritable;
 import io.helidon.common.http.Http;
 
-import org.hamcrest.core.Is;
-import org.hamcrest.core.StringEndsWith;
-
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.notNullValue;
 import static org.hamcrest.CoreMatchers.startsWith;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.core.Is.is;
 
 /**
  * The SocketHttpClient provides means to simply pass any bytes over the network
  * and to see how a server deals with such a case.
  */
-public class SocketHttpClient {
+public class SocketHttpClient implements AutoCloseable {
 
     private static final System.Logger LOGGER = System.getLogger(SocketHttpClient.class.getName());
 
     private static final String EOL = "\r\n";
     private static final Pattern FIRST_LINE_PATTERN = Pattern.compile("HTTP/\\d+\\.\\d+ (\\d\\d\\d) (.*)");
     private final String host;
     private final int port;
+    private final Duration timeout;
 
-    private Duration timeout;
     private Socket socket;
     private boolean connected;
 
-    /**
-     * Creates the instance linked with the provided server.
-     *
-     * @param host    host to connect to
-     * @param port    port to connect to
-     * @param timeout to use when connecting
-     */
-    public SocketHttpClient(String host, int port, Duration timeout) {
+    protected SocketHttpClient(String host, int port, Duration timeout) {
         this.host = host;
         this.port = port;
         this.timeout = timeout;
     }
 
     /**
-     * Creates the instance linked with the provided server.
-     * Timeout is set to 5 seconds.
+     * Socket client that allows sending any content.
      *
-     * @param host host to connect to
-     * @param port port to connect to
+     * @param host    host to connect to
+     * @param port    port to connect to
+     * @param timeout socket timeout
+     * @return a new (disconnected) client
      */
-    public SocketHttpClient(String host, int port) {
-        this(host, port, Duration.of(5, ChronoUnit.SECONDS));
+    public static SocketHttpClient create(String host, int port, Duration timeout) {
+        return new SocketHttpClient(host, port, timeout);
     }
 
     /**
-     * Assert that the socket associated with the provided client is working and open.
+     * Socket client that allows sending any content.
+     * Uses localhost and timeout of 5 seconds.
      *
-     * @param s the socket client
+     * @param port    port to connect to
+     * @return a new (disconnected) client
      */
-    public static void assertConnectionIsOpen(SocketHttpClient s) {
-        // get
-        s.request(Http.Method.GET);
-        // assert
-        assertThat(s.receive(), StringEndsWith.endsWith("\n9\nIt works!\n0\n\n"));
-    }
-
-    /**
-     * Assert that the socket associated with the provided client is closed.
-     *
-     * @param s the socket client
-     */
-    public static void assertConnectionIsClosed(SocketHttpClient s) {
-        // get
-        s.request(Http.Method.POST, null);
-        // assert
-        try {
-            // when the connection is closed before we start reading, just "" is returned by receive()
-            assertThat(s.receive(), Is.is(""));
-        } catch (UncheckedIOException e) {
-            if (e.getCause() instanceof SocketException) {
-                // "Connection reset" exception is thrown in case we were fast enough and started receiving the response
-                // before it was closed
-                LOGGER.log(System.Logger.Level.TRACE, "Received: " + e.getMessage());
-            } else {
-                throw e;
-            }
-        }
+    public static SocketHttpClient create(int port) {
+        return create("localhost", port, Duration.ofSeconds(5));
     }
 
     /**
@@ -143,7 +110,8 @@ public static StringBuilder longData(int bytes) {
      * @param response full HTTP response
      * @return headers map
      */
-    public static Map<String, String> headersFromResponse(String response) {
+    public static HeadersClientResponse headersFromResponse(String response) {
+        HeadersWritable<?> headers = HeadersWritable.create();
 
         assertThat(response, notNullValue());
         int index = response.indexOf("\n\n");
@@ -153,9 +121,9 @@ public static Map<String, String> headersFromResponse(String response) {
         String hdrsPart = response.substring(0, index);
         String[] lines = hdrsPart.split("\\n");
         if (lines.length <= 1) {
-            return Collections.emptyMap();
+            return HeadersClientResponse.create(headers);
         }
-        Map<String, String> result = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
+
         boolean first = true;
         for (String line : lines) {
             if (first) {
@@ -166,9 +134,9 @@ public static Map<String, String> headersFromResponse(String response) {
             if (i < 0) {
                 throw new AssertionError("Header without semicolon - " + line);
             }
-            result.put(line.substring(0, i).trim(), line.substring(i + 1).trim());
+            headers.add(Http.Header.create(line.substring(0, i).trim()), line.substring(i + 1).trim());
         }
-        return result;
+        return HeadersClientResponse.create(headers);
     }
 
     /**
@@ -218,6 +186,42 @@ public static String entityFromResponse(String response, boolean validateHeaderF
         return response.substring(index + 2);
     }
 
+    @Override
+    public void close() throws Exception {
+        disconnect();
+    }
+
+    /**
+     * Assert that the socket is working and open.
+     */
+    public void assertConnectionIsOpen() {
+        // get
+        request(Http.Method.GET, "/this/path/should/not/exist", null);
+        // assert
+        assertThat(receive(), containsString("HTTP/1.1"));
+    }
+
+    /**
+     * Assert that the socket is closed.
+     */
+    public void assertConnectionIsClosed() {
+        // get
+        request(Http.Method.POST, null);
+        // assert
+        try {
+            // when the connection is closed before we start reading, just "" is returned by receive()
+            assertThat(receive(), is(""));
+        } catch (UncheckedIOException e) {
+            if (e.getCause() instanceof SocketException) {
+                // "Connection reset" exception is thrown in case we were fast enough and started receiving the response
+                // before it was closed
+                LOGGER.log(System.Logger.Level.TRACE, "Received: " + e.getMessage());
+            } else {
+                throw e;
+            }
+        }
+    }
+
     /**
      * A helper method that sends the given payload with the provided method to the server.
      *

diff --git a/...don/common/testing/http/package-info.java → ...mon/testing/http/junit5/package-info.java b/...don/common/testing/http/package-info.java → ...mon/testing/http/junit5/package-info.java
@@ -19,7 +19,7 @@
  * <p>
  * Hamcrest Matchers:
  * <ul>
- *     <li>{@link io.helidon.common.testing.http.HttpHeaderMatcher}</li>
+ *     <li>{@link io.helidon.common.testing.http.junit5.HttpHeaderMatcher}</li>
  * </ul>
  */
-package io.helidon.common.testing.http;
+package io.helidon.common.testing.http.junit5;
diff --git a/common/testing/http-junit5/src/main/java/module-info.java b/common/testing/http-junit5/src/main/java/module-info.java
@@ -23,5 +23,5 @@
     requires hamcrest.all;
     requires org.junit.jupiter.api;
 
-    exports io.helidon.common.testing.http;
+    exports io.helidon.common.testing.http.junit5;
 }
diff --git a/...ount-se/src/main/java/io/helidon/examples/se/httpstatuscount/HttpStatusMetricService.java b/...ount-se/src/main/java/io/helidon/examples/se/httpstatuscount/HttpStatusMetricService.java
@@ -15,6 +15,8 @@
  */
 package io.helidon.examples.se.httpstatuscount;
 
+import java.util.concurrent.atomic.AtomicInteger;
+
 import io.helidon.metrics.api.RegistryFactory;
 import io.helidon.reactive.webserver.Routing;
 import io.helidon.reactive.webserver.ServerRequest;
@@ -43,6 +45,8 @@ public class HttpStatusMetricService implements Service {
 
     static final String STATUS_TAG_NAME = "range";
 
+    private static final AtomicInteger IN_PROGRESS = new AtomicInteger();
+
     private final Counter[] responseCounters = new Counter[6];
 
     static HttpStatusMetricService create() {
@@ -69,8 +73,14 @@ public void update(Routing.Rules rules) {
         rules.any(this::updateRange);
     }
 
+    // for testing
+    static boolean isInProgress() {
+        return IN_PROGRESS.get() != 0;
+    }
+
     // Edited to adopt Ciaran's fix later in the thread.
     private void updateRange(ServerRequest request, ServerResponse response) {
+        IN_PROGRESS.incrementAndGet();
         response.whenSent()
                 .thenAccept(this::logMetric);
         request.next();
@@ -81,5 +91,6 @@ private void logMetric(ServerResponse response) {
         if (range > 0 && range < responseCounters.length) {
             responseCounters[range].inc();
         }
+        IN_PROGRESS.decrementAndGet();
     }
 }