8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERROR: Shutting down connection: HTTP/2 client stopped"

Reviewed-by: jpai

test/jdk/java/net/httpclient/HttpServerAdapters.java

@@ -45,6 +45,7 @@ import java.net.http.HttpHeaders;
 import java.util.List;
 import java.util.ListIterator;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -105,6 +106,16 @@ public interface HttpServerAdapters {
         public abstract Set<Map.Entry<String, List<String>>> entrySet();
         public abstract List<String> get(String name);
         public abstract boolean containsKey(String name);
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) return true;
+            if (!(o instanceof HttpTestRequestHeaders other)) return false;
+            return Objects.equals(entrySet(), other.entrySet());
+        }
+        @Override
+        public int hashCode() {
+            return Objects.hashCode(entrySet());
+        }
 
         public static HttpTestRequestHeaders of(Headers headers) {
             return new Http1TestRequestHeaders(headers);
@@ -138,6 +149,10 @@ public interface HttpServerAdapters {
             public boolean containsKey(String name) {
                 return headers.containsKey(name);
             }
+            @Override
+            public String toString() {
+                return String.valueOf(headers);
+            }
         }
         private static final class Http2TestRequestHeaders extends HttpTestRequestHeaders {
             private final HttpHeaders headers;
@@ -160,6 +175,10 @@ public interface HttpServerAdapters {
             public boolean containsKey(String name) {
                 return headers.firstValue(name).isPresent();
             }
+            @Override
+            public String toString() {
+                return String.valueOf(headers);
+            }
         }
     }
 

test/jdk/java/net/httpclient/SpecialHeadersTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -47,7 +47,12 @@ import com.sun.net.httpserver.HttpServer;
 import com.sun.net.httpserver.HttpsConfigurator;
 import com.sun.net.httpserver.HttpsServer;
 import jdk.test.lib.net.SimpleSSLContext;
+import org.testng.ITestContext;
+import org.testng.ITestResult;
+import org.testng.SkipException;
+import org.testng.annotations.AfterClass;
 import org.testng.annotations.AfterTest;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.BeforeTest;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
@@ -70,8 +75,20 @@ import java.util.Arrays;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import static java.lang.System.err;
 import static java.lang.System.out;
 import static java.net.http.HttpClient.Builder.NO_PROXY;
 import static java.net.http.HttpClient.Version.HTTP_2;
@@ -140,6 +157,142 @@ public class SpecialHeadersTest implements HttpServerAdapters {
     }
 
     static final int ITERATION_COUNT = 3; // checks upgrade and re-use
+    // a shared executor helps reduce the amount of threads created by the test
+    static final TestExecutor executor = new TestExecutor(Executors.newCachedThreadPool());
+    static final ConcurrentMap<String, Throwable> FAILURES = new ConcurrentHashMap<>();
+    static volatile boolean tasksFailed;
+    static final AtomicLong serverCount = new AtomicLong();
+    static final AtomicLong clientCount = new AtomicLong();
+    static final long start = System.nanoTime();
+    public static String now() {
+        long now = System.nanoTime() - start;
+        long secs = now / 1000_000_000;
+        long mill = (now % 1000_000_000) / 1000_000;
+        long nan = now % 1000_000;
+        return String.format("[%d s, %d ms, %d ns] ", secs, mill, nan);
+    }
+
+    final ReferenceTracker TRACKER = ReferenceTracker.INSTANCE;
+    private volatile HttpClient sharedClient;
+
+    static class TestExecutor implements Executor {
+        final AtomicLong tasks = new AtomicLong();
+        Executor executor;
+        TestExecutor(Executor executor) {
+            this.executor = executor;
+        }
+
+        @Override
+        public void execute(Runnable command) {
+            long id = tasks.incrementAndGet();
+            executor.execute(() -> {
+                try {
+                    command.run();
+                } catch (Throwable t) {
+                    tasksFailed = true;
+                    out.printf(now() + "Task %s failed: %s%n", id, t);
+                    err.printf(now() + "Task %s failed: %s%n", id, t);
+                    FAILURES.putIfAbsent("Task " + id, t);
+                    throw t;
+                }
+            });
+        }
+
+        public void shutdown() throws InterruptedException {
+            if (executor instanceof ExecutorService service) {
+                service.shutdown();
+                service.awaitTermination(1000, TimeUnit.MILLISECONDS);
+            }
+        }
+
+
+    }
+
+    protected boolean stopAfterFirstFailure() {
+        return Boolean.getBoolean("jdk.internal.httpclient.debug");
+    }
+
+    final AtomicReference<SkipException> skiptests = new AtomicReference<>();
+    void checkSkip() {
+        var skip = skiptests.get();
+        if (skip != null) throw skip;
+    }
+    static String name(ITestResult result) {
+        var params = result.getParameters();
+        return result.getName()
+                + (params == null ? "()" : Arrays.toString(result.getParameters()));
+    }
+
+    @BeforeMethod
+    void beforeMethod(ITestContext context) {
+        if (stopAfterFirstFailure() && context.getFailedTests().size() > 0) {
+            if (skiptests.get() == null) {
+                SkipException skip = new SkipException("some tests failed");
+                skip.setStackTrace(new StackTraceElement[0]);
+                skiptests.compareAndSet(null, skip);
+            }
+        }
+    }
+
+    @AfterClass
+    static final void printFailedTests(ITestContext context) {
+        out.println("\n=========================");
+        var failed = context.getFailedTests().getAllResults().stream()
+                .collect(Collectors.toMap(r -> name(r), ITestResult::getThrowable));
+        FAILURES.putAll(failed);
+        try {
+            out.printf("%n%sCreated %d servers and %d clients%n",
+                    now(), serverCount.get(), clientCount.get());
+            if (FAILURES.isEmpty()) return;
+            out.println("Failed tests: ");
+            FAILURES.entrySet().forEach((e) -> {
+                out.printf("\t%s: %s%n", e.getKey(), e.getValue());
+                e.getValue().printStackTrace(out);
+            });
+            if (tasksFailed) {
+                out.println("WARNING: Some tasks failed");
+            }
+        } finally {
+            out.println("\n=========================\n");
+        }
+    }
+
+    private HttpClient makeNewClient() {
+        clientCount.incrementAndGet();
+        return HttpClient.newBuilder()
+                .proxy(NO_PROXY)
+                .executor(executor)
+                .sslContext(sslContext)
+                .build();
+    }
+
+    static volatile String lastMethod;
+    HttpClient newHttpClient(String method, boolean share) {
+        if (!share) return TRACKER.track(makeNewClient());
+        HttpClient shared = sharedClient;
+        String last = lastMethod;
+        if (shared != null && Objects.equals(last, method)) return shared;
+        synchronized (this) {
+            shared = sharedClient;
+            last = lastMethod;
+            if (!Objects.equals(last, method)) {
+                // reset sharedClient to avoid side effects
+                // between methods. This is needed to keep the test
+                // expectation that the first HTTP/2 clear request
+                // will be an upgrade
+                if (shared != null) {
+                    TRACKER.track(shared);
+                    shared = sharedClient = null;
+                }
+            }
+            if (shared == null) {
+                shared = sharedClient = makeNewClient();
+                last = lastMethod = method;
+            }
+            return shared;
+        }
+    }
+
 
     static String userAgent() {
         return "Java-http-client/" + System.getProperty("java.version");
@@ -167,48 +320,54 @@ public class SpecialHeadersTest implements HttpServerAdapters {
 
         HttpClient client = null;
         for (int i=0; i< ITERATION_COUNT; i++) {
-            if (!sameClient || client == null)
+            try {
-                client = HttpClient.newBuilder()
+                if (!sameClient || client == null)
-                        .proxy(NO_PROXY)
+                    client = newHttpClient("test", sameClient);
-                        .sslContext(sslContext)
-                        .build();
 
-            HttpRequest.Builder requestBuilder = HttpRequest.newBuilder(uri);
+                HttpRequest.Builder requestBuilder = HttpRequest.newBuilder(uri);
-            if (!useDefault) {
+                if (!useDefault) {
-                requestBuilder.header(name, value);
+                    requestBuilder.header(name, value);
-            }
+                }
-            HttpRequest request = requestBuilder.build();
+                HttpRequest request = requestBuilder.build();
-            HttpResponse<String> resp = client.send(request, BodyHandlers.ofString());
+                HttpResponse<String> resp = client.send(request, BodyHandlers.ofString());
 
-            out.println("Got response: " + resp);
+                out.println("Got response: " + resp);
-            out.println("Got body: " + resp.body());
+                out.println("Got body: " + resp.body());
-            assertEquals(resp.statusCode(), 200,
+                assertEquals(resp.statusCode(), 200,
-                    "Expected 200, got:" + resp.statusCode());
+                        "Expected 200, got:" + resp.statusCode());
 
-            boolean isInitialRequest = i == 0;
+                boolean isInitialRequest = i == 0;
-            boolean isSecure = uri.getScheme().equalsIgnoreCase("https");
+                boolean isSecure = uri.getScheme().equalsIgnoreCase("https");
-            boolean isHTTP2 = resp.version() == HTTP_2;
+                boolean isHTTP2 = resp.version() == HTTP_2;
-            boolean isNotH2CUpgrade = isSecure || (sameClient == true && !isInitialRequest);
+                boolean isNotH2CUpgrade = isSecure || (sameClient == true && !isInitialRequest);
-            boolean isDefaultHostHeader = name.equalsIgnoreCase("host") && useDefault;
+                boolean isDefaultHostHeader = name.equalsIgnoreCase("host") && useDefault;
 
-            // By default, HTTP/2 sets the `:authority:` pseudo-header, instead
+                // By default, HTTP/2 sets the `:authority:` pseudo-header, instead
-            // of the `Host` header. Therefore, there should be no "X-Host"
+                // of the `Host` header. Therefore, there should be no "X-Host"
-            // header in the response, except the response to the h2c Upgrade
+                // header in the response, except the response to the h2c Upgrade
-            // request which will have been sent through HTTP/1.1.
+                // request which will have been sent through HTTP/1.1.
 
-            if (isDefaultHostHeader && isHTTP2 && isNotH2CUpgrade) {
+                if (isDefaultHostHeader && isHTTP2 && isNotH2CUpgrade) {
-                assertTrue(resp.headers().firstValue("X-" + key).isEmpty());
+                    assertTrue(resp.headers().firstValue("X-" + key).isEmpty());
-                assertTrue(resp.headers().allValues("X-" + key).isEmpty());
+                    assertTrue(resp.headers().allValues("X-" + key).isEmpty());
-                out.println("No X-" + key + " header received, as expected");
+                    out.println("No X-" + key + " header received, as expected");
-            } else {
-                String receivedHeaderString = value == null ? null
-                        : resp.headers().firstValue("X-"+key).get();
-                out.println("Got X-" + key + ": " + resp.headers().allValues("X-"+key));
-                if (value != null) {
-                    assertEquals(receivedHeaderString, value);
-                    assertEquals(resp.headers().allValues("X-"+key), List.of(value));
                 } else {
-                    assertEquals(resp.headers().allValues("X-"+key).size(), 0);
+                    String receivedHeaderString = value == null ? null
+                            : resp.headers().firstValue("X-" + key).orElse(null);
+                    out.println("Got X-" + key + ": " + resp.headers().allValues("X-" + key));
+                    if (value != null) {
+                        assertEquals(receivedHeaderString, value);
+                        assertEquals(resp.headers().allValues("X-" + key), List.of(value));
+                    } else {
+                        assertEquals(resp.headers().allValues("X-" + key).size(), 0);
+                    }
+                }
+            } finally {
+                if (!sameClient) {
+                    client = null;
+                    System.gc();
+                    var error = TRACKER.check(500);
+                    if (error != null) throw error;
                 }
             }
         }
@@ -223,42 +382,61 @@ public class SpecialHeadersTest implements HttpServerAdapters {
         out.println("\n--- Starting ");
         final URI uri = URI.create(uriString);
 
-        HttpClient client = HttpClient.newBuilder()
+        HttpClient client = newHttpClient("testHomeMadeIllegalHeader", sameClient);
-                .proxy(NO_PROXY)
-                .sslContext(sslContext)
-                .build();
-
-        // Test a request which contains an illegal header created
-        HttpRequest req = new HttpRequest() {
-            @Override public Optional<BodyPublisher> bodyPublisher() {
-                return Optional.of(BodyPublishers.noBody());
-            }
-            @Override public String method() {
-                return "GET";
-            }
-            @Override public Optional<Duration> timeout() {
-                return Optional.empty();
-            }
-            @Override public boolean expectContinue() {
-                return false;
-            }
-            @Override public URI uri() {
-                return uri;
-            }
-            @Override public Optional<HttpClient.Version> version() {
-                return Optional.empty();
-            }
-            @Override public HttpHeaders headers() {
-                Map<String, List<String>> map = Map.of("upgrade", List.of("http://foo.com"));
-                return HttpHeaders.of(map, (x, y) -> true);
-            }
-        };
 
         try {
-            HttpResponse<String> response = client.send(req, BodyHandlers.ofString());
+            // Test a request which contains an illegal header created
-            Assert.fail("Unexpected reply: " + response);
+            HttpRequest req = new HttpRequest() {
-        } catch (IllegalArgumentException ee) {
+                @Override
-            out.println("Got IAE as expected");
+                public Optional<BodyPublisher> bodyPublisher() {
+                    return Optional.of(BodyPublishers.noBody());
+                }
+
+                @Override
+                public String method() {
+                    return "GET";
+                }
+
+                @Override
+                public Optional<Duration> timeout() {
+                    return Optional.empty();
+                }
+
+                @Override
+                public boolean expectContinue() {
+                    return false;
+                }
+
+                @Override
+                public URI uri() {
+                    return uri;
+                }
+
+                @Override
+                public Optional<HttpClient.Version> version() {
+                    return Optional.empty();
+                }
+
+                @Override
+                public HttpHeaders headers() {
+                    Map<String, List<String>> map = Map.of("upgrade", List.of("http://foo.com"));
+                    return HttpHeaders.of(map, (x, y) -> true);
+                }
+            };
+
+            try {
+                HttpResponse<String> response = client.send(req, BodyHandlers.ofString());
+                Assert.fail("Unexpected reply: " + response);
+            } catch (IllegalArgumentException ee) {
+                out.println("Got IAE as expected");
+            }
+        } finally {
+            if (!sameClient) {
+                client = null;
+                System.gc();
+                var error = TRACKER.check(500);
+                if (error != null) throw error;
+            }
         }
     }
 
@@ -276,52 +454,59 @@ public class SpecialHeadersTest implements HttpServerAdapters {
 
         HttpClient client = null;
         for (int i=0; i< ITERATION_COUNT; i++) {
-            if (!sameClient || client == null)
+            try {
-                client = HttpClient.newBuilder()
+                if (!sameClient || client == null)
-                        .proxy(NO_PROXY)
+                    client = newHttpClient("testAsync", sameClient);
-                        .sslContext(sslContext)
-                        .build();
 
-            HttpRequest.Builder requestBuilder = HttpRequest.newBuilder(uri);
+                HttpRequest.Builder requestBuilder = HttpRequest.newBuilder(uri);
-            if (!useDefault) {
+                if (!useDefault) {
-                requestBuilder.header(name, value);
+                    requestBuilder.header(name, value);
-            }
+                }
-            HttpRequest request = requestBuilder.build();
+                HttpRequest request = requestBuilder.build();
 
-            boolean isInitialRequest = i == 0;
+                boolean isInitialRequest = i == 0;
-            boolean isSecure = uri.getScheme().equalsIgnoreCase("https");
+                boolean isSecure = uri.getScheme().equalsIgnoreCase("https");
-            boolean isNotH2CUpgrade = isSecure || (sameClient == true && !isInitialRequest);
+                boolean isNotH2CUpgrade = isSecure || (sameClient == true && !isInitialRequest);
-            boolean isDefaultHostHeader = name.equalsIgnoreCase("host") && useDefault;
+                boolean isDefaultHostHeader = name.equalsIgnoreCase("host") && useDefault;
 
-            client.sendAsync(request, BodyHandlers.ofString())
+                client.sendAsync(request, BodyHandlers.ofString())
-                    .thenApply(response -> {
+                        .thenApply(response -> {
-                        out.println("Got response: " + response);
+                            out.println("Got response: " + response);
-                        out.println("Got body: " + response.body());
+                            out.println("Got body: " + response.body());
-                        assertEquals(response.statusCode(), 200);
+                            assertEquals(response.statusCode(), 200);
-                        return response;})
+                            return response;
-                    .thenAccept(resp -> {
+                        })
-                        // By default, HTTP/2 sets the `:authority:` pseudo-header, instead
+                        .thenAccept(resp -> {
-                        // of the `Host` header. Therefore, there should be no "X-Host"
+                            // By default, HTTP/2 sets the `:authority:` pseudo-header, instead
-                        // header in the response, except the response to the h2c Upgrade
+                            // of the `Host` header. Therefore, there should be no "X-Host"
-                        // request which will have been sent through HTTP/1.1.
+                            // header in the response, except the response to the h2c Upgrade
+                            // request which will have been sent through HTTP/1.1.
 
-                        if (isDefaultHostHeader && resp.version() == HTTP_2 && isNotH2CUpgrade) {
+                            if (isDefaultHostHeader && resp.version() == HTTP_2 && isNotH2CUpgrade) {
-                            assertTrue(resp.headers().firstValue("X-" + key).isEmpty());
+                                assertTrue(resp.headers().firstValue("X-" + key).isEmpty());
-                            assertTrue(resp.headers().allValues("X-" + key).isEmpty());
+                                assertTrue(resp.headers().allValues("X-" + key).isEmpty());
-                            out.println("No X-" + key + " header received, as expected");
+                                out.println("No X-" + key + " header received, as expected");
-                        } else {
-                            String receivedHeaderString = value == null ? null
-                                    : resp.headers().firstValue("X-"+key).get();
-                            out.println("Got X-" + key + ": " + resp.headers().allValues("X-"+key));
-                            if (value != null) {
-                                assertEquals(receivedHeaderString, value);
-                                assertEquals(resp.headers().allValues("X-" + key), List.of(value));
                             } else {
-                                assertEquals(resp.headers().allValues("X-" + key).size(), 1);
+                                String receivedHeaderString = value == null ? null
+                                        : resp.headers().firstValue("X-" + key).orElse(null);
+                                out.println("Got X-" + key + ": " + resp.headers().allValues("X-" + key));
+                                if (value != null) {
+                                    assertEquals(receivedHeaderString, value);
+                                    assertEquals(resp.headers().allValues("X-" + key), List.of(value));
+                                } else {
+                                    assertEquals(resp.headers().allValues("X-" + key).size(), 1);
+                                }
                             }
-                        }
+                        })
-                    })
+                        .join();
-                    .join();
+            } finally {
+                if (!sameClient) {
+                    client = null;
+                    System.gc();
+                    var error = TRACKER.check(500);
+                    if (error != null) throw error;
+                }
+            }
         }
     }
 
@@ -364,10 +549,27 @@ public class SpecialHeadersTest implements HttpServerAdapters {
 
     @AfterTest
     public void teardown() throws Exception {
-        httpTestServer.stop();
+        HttpClient shared = sharedClient;
-        httpsTestServer.stop();
+        String sharedClientName =
-        http2TestServer.stop();
+                shared == null ? null : shared.toString();
-        https2TestServer.stop();
+        if (shared != null) TRACKER.track(shared);
+        shared = sharedClient = null;
+        Thread.sleep(100);
+        AssertionError fail = TRACKER.check(500);
+        try {
+            httpTestServer.stop();
+            httpsTestServer.stop();
+            http2TestServer.stop();
+            https2TestServer.stop();
+            executor.shutdown();
+        } finally {
+            if (fail != null) {
+                if (sharedClientName != null) {
+                    err.println("Shared client name is: " + sharedClientName);
+                }
+                throw fail;
+            }
+        }
     }
 
     /** A handler that returns, as its body, the exact received request URI.
@@ -379,9 +581,11 @@ public class SpecialHeadersTest implements HttpServerAdapters {
         public void handle(HttpTestExchange t) throws IOException {
             URI uri = t.getRequestURI();
             String uriString = uri.toString();
-            out.println("Http1UriStringHandler received, uri: " + uriString);
+            out.println("HttpUriStringHandler received, uri: " + uriString);
             String query = uri.getQuery();
             String headerName = query.substring(query.indexOf("=")+1).trim();
+            out.println("HttpUriStringHandler received, headerName: " + headerName
+                    + "\n\theaders: " + t.getRequestHeaders());
             try (InputStream is = t.getRequestBody();
                  OutputStream os = t.getResponseBody()) {
                 is.readAllBytes();