http-client-branch: fix HTTP/1.1 retry to avoid duplication of header values http-client-branch
authordfuchs
Fri, 23 Mar 2018 18:01:52 +0000
branchhttp-client-branch
changeset 56345 eb72d194235c
parent 56344 2f3c34e2b691
child 56346 514c68575523
http-client-branch: fix HTTP/1.1 retry to avoid duplication of header values
src/java.net.http/share/classes/jdk/internal/net/http/MultiExchange.java
src/java.net.http/share/classes/jdk/internal/net/http/ProxyAuthenticationRequired.java
src/java.net.http/share/classes/jdk/internal/net/http/SocketTube.java
test/jdk/java/net/httpclient/RetryWithCookie.java
--- a/src/java.net.http/share/classes/jdk/internal/net/http/MultiExchange.java	Thu Mar 22 16:13:36 2018 +0000
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/MultiExchange.java	Fri Mar 23 18:01:52 2018 +0000
@@ -71,7 +71,8 @@
     final HttpResponse.BodyHandler<T> responseHandler;
     final Executor executor;
     final AtomicInteger attempts = new AtomicInteger();
-    HttpRequestImpl currentreq; // used for async only
+    HttpRequestImpl currentreq; // used for retries & redirect
+    HttpRequestImpl previousreq; // used for retries & redirect
     Exchange<T> exchange; // the current exchange
     Exchange<T> previous;
     volatile Throwable retryCause;
@@ -114,6 +115,7 @@
         this.userRequest = userRequest;
         this.request = requestImpl;
         this.currentreq = request;
+        this.previousreq = null;
         this.client = client;
         this.filters = client.filterChain();
         this.acc = acc;
@@ -186,11 +188,6 @@
         return null;
     }
 
-//    public void cancel() {
-//        cancelled = true;
-//        getExchange().cancel();
-//    }
-
     public void cancel(IOException cause) {
         cancelled = true;
         getExchange().cancel(cause);
@@ -228,7 +225,13 @@
             }
             try {
                 // 1. apply request filters
-                requestFilters(currentreq);
+                // if currentreq == previousreq the filters have already
+                // been applied once. Applying them a second time might
+                // cause some headers values to be added twice: for
+                // instance, the same cookie might be added again.
+                if (currentreq != previousreq) {
+                    requestFilters(currentreq);
+                }
             } catch (IOException e) {
                 return failedFuture(e);
             }
@@ -254,6 +257,7 @@
                                 new HttpResponseImpl<>(currentreq, response, this.response, null, exch);
                             Exchange<T> oldExch = exch;
                             return exch.ignoreBody().handle((r,t) -> {
+                                previousreq = currentreq;
                                 currentreq = newrequest;
                                 expiredOnce = false;
                                 setExchange(new Exchange<>(currentreq, this, acc));
@@ -300,6 +304,12 @@
                     "ConnectionExpiredException (async): retrying...",
                     t);
                 expiredOnce = true;
+                // The connection was abruptly closed.
+                // We return null to retry the same request a second time.
+                // The request filters have already been applied to the
+                // currentreq, so we set previousreq = currentreq to
+                // prevent them from being applied again.
+                previousreq = currentreq;
                 return null;
             } else {
                 DEBUG_LOGGER.log(Level.DEBUG,
--- a/src/java.net.http/share/classes/jdk/internal/net/http/ProxyAuthenticationRequired.java	Thu Mar 22 16:13:36 2018 +0000
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/ProxyAuthenticationRequired.java	Fri Mar 23 18:01:52 2018 +0000
@@ -36,7 +36,7 @@
     final transient Response proxyResponse;
 
     /**
-     * Constructs a {@code ConnectionExpiredException} with the specified detail
+     * Constructs a {@code ProxyAuthenticationRequired} with the specified detail
      * message and cause.
      *
      * @param   proxyResponse the response from the proxy
--- a/src/java.net.http/share/classes/jdk/internal/net/http/SocketTube.java	Thu Mar 22 16:13:36 2018 +0000
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/SocketTube.java	Fri Mar 23 18:01:52 2018 +0000
@@ -305,8 +305,10 @@
 
         @Override
         public void onNext(List<ByteBuffer> bufs) {
-            assert current == null; // this is a queue of 1.
-            assert subscription != null;
+            assert current == null : dbgString() // this is a queue of 1.
+                    + "w.onNext current: " + current;
+            assert subscription != null : dbgString()
+                    + "w.onNext: subscription is null";
             current = bufs;
             tryFlushCurrent(client.isSelectorThread()); // may be in selector thread
             // For instance in HTTP/2, a received SETTINGS frame might trigger
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/net/httpclient/RetryWithCookie.java	Fri Mar 23 18:01:52 2018 +0000
@@ -0,0 +1,258 @@
+/*
+ * Copyright (c) 2018, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8199943
+ * @summary Test for cookie handling when retrying after close
+ * @modules java.base/sun.net.www.http
+ *          java.net.http/jdk.internal.net.http.common
+ *          java.net.http/jdk.internal.net.http.frame
+ *          java.net.http/jdk.internal.net.http.hpack
+ *          java.logging
+ *          jdk.httpserver
+ * @library /lib/testlibrary /test/lib http2/server
+ * @build Http2TestServer
+ * @build jdk.testlibrary.SimpleSSLContext
+ * @run testng/othervm
+ *       -Djdk.httpclient.HttpClient.log=trace,headers,requests
+ *       RetryWithCookie
+ */
+
+import com.sun.net.httpserver.HttpServer;
+import com.sun.net.httpserver.HttpsConfigurator;
+import com.sun.net.httpserver.HttpsServer;
+import jdk.testlibrary.SimpleSSLContext;
+import org.testng.annotations.AfterTest;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import javax.net.ssl.SSLContext;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.net.CookieManager;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.URI;
+import java.net.http.HttpClient;
+import java.net.http.HttpClient.Redirect;
+import java.net.http.HttpRequest;
+import java.net.http.HttpResponse;
+import java.net.http.HttpResponse.BodyHandlers;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+
+import static java.lang.System.out;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+public class RetryWithCookie implements HttpServerAdapters {
+
+    SSLContext sslContext;
+    HttpTestServer httpTestServer;        // HTTP/1.1    [ 4 servers ]
+    HttpTestServer httpsTestServer;       // HTTPS/1.1
+    HttpTestServer http2TestServer;       // HTTP/2 ( h2c )
+    HttpTestServer https2TestServer;      // HTTP/2 ( h2  )
+    String httpURI;
+    String httpsURI;
+    String http2URI;
+    String https2URI;
+
+    static final String MESSAGE = "BasicRedirectTest message body";
+    static final int ITERATIONS = 3;
+
+    @DataProvider(name = "positive")
+    public Object[][] positive() {
+        return new Object[][] {
+                { httpURI,    },
+                { httpsURI,   },
+                { http2URI,   },
+                { https2URI,  },
+        };
+    }
+
+    static final AtomicLong requestCounter = new AtomicLong();
+
+    @Test(dataProvider = "positive")
+    void test(String uriString) throws Exception {
+        out.printf("%n---- starting (%s) ----%n", uriString);
+        CookieManager cookieManager = new CookieManager();
+        HttpClient client = HttpClient.newBuilder()
+                .followRedirects(Redirect.ALWAYS)
+                .cookieHandler(cookieManager)
+                .sslContext(sslContext)
+                .build();
+        assert client.cookieHandler().isPresent();
+
+        URI uri = URI.create(uriString);
+        List<String> cookies = new ArrayList<>();
+        cookies.add("CUSTOMER=ARTHUR_DENT");
+        Map<String, List<String>> cookieHeaders = new HashMap<>();
+        cookieHeaders.put("Set-Cookie", cookies);
+        cookieManager.put(uri, cookieHeaders);
+
+        HttpRequest request = HttpRequest.newBuilder(uri)
+                .header("X-uuid", "uuid-" + requestCounter.incrementAndGet())
+                .build();
+        out.println("Initial request: " + request.uri());
+
+        for (int i=0; i< ITERATIONS; i++) {
+            out.println("iteration: " + i);
+            HttpResponse<String> response = client.send(request, BodyHandlers.ofString());
+
+            out.println("  Got response: " + response);
+            out.println("  Got body Path: " + response.body());
+
+            assertEquals(response.statusCode(), 200);
+            assertEquals(response.body(), MESSAGE);
+            assertEquals(response.headers().allValues("X-Request-Cookie"), cookies);
+            request = HttpRequest.newBuilder(uri)
+                    .header("X-uuid", "uuid-" + requestCounter.incrementAndGet())
+                    .build();
+        }
+    }
+
+    // -- Infrastructure
+
+    @BeforeTest
+    public void setup() throws Exception {
+        sslContext = new SimpleSSLContext().get();
+        if (sslContext == null)
+            throw new AssertionError("Unexpected null sslContext");
+
+        InetSocketAddress sa = new InetSocketAddress(InetAddress.getLoopbackAddress(), 0);
+
+        httpTestServer = HttpTestServer.of(HttpServer.create(sa, 0));
+        httpTestServer.addHandler(new CookieRetryHandler(), "/http1/cookie/");
+        httpURI = "http://" + httpTestServer.serverAuthority() + "/http1/cookie/retry";
+        HttpsServer httpsServer = HttpsServer.create(sa, 0);
+        httpsServer.setHttpsConfigurator(new HttpsConfigurator(sslContext));
+        httpsTestServer = HttpTestServer.of(httpsServer);
+        httpsTestServer.addHandler(new CookieRetryHandler(),"/https1/cookie/");
+        httpsURI = "https://" + httpsTestServer.serverAuthority() + "/https1/cookie/retry";
+
+        http2TestServer = HttpTestServer.of(new Http2TestServer("localhost", false, 0));
+        http2TestServer.addHandler(new CookieRetryHandler(), "/http2/cookie/");
+        http2URI = "http://" + http2TestServer.serverAuthority() + "/http2/cookie/retry";
+        https2TestServer = HttpTestServer.of(new Http2TestServer("localhost", true, 0));
+        https2TestServer.addHandler(new CookieRetryHandler(), "/https2/cookie/");
+        https2URI = "https://" + https2TestServer.serverAuthority() + "/https2/cookie/retry";
+
+        httpTestServer.start();
+        httpsTestServer.start();
+        http2TestServer.start();
+        https2TestServer.start();
+    }
+
+    @AfterTest
+    public void teardown() throws Exception {
+        httpTestServer.stop();
+        httpsTestServer.stop();
+        http2TestServer.stop();
+        https2TestServer.stop();
+    }
+
+    static class CookieRetryHandler implements HttpTestHandler {
+        ConcurrentHashMap<String,String> closedRequests = new ConcurrentHashMap<>();
+
+        @Override
+        public void handle(HttpTestExchange t) throws IOException {
+            System.out.println("CookieRetryHandler for: " + t.getRequestURI());
+
+            List<String> uuids = t.getRequestHeaders().get("X-uuid");
+            if (uuids == null || uuids.size() != 1) {
+                readAllRequestData(t);
+                try (OutputStream os = t.getResponseBody()) {
+                    String msg = "Incorrect uuid header values:[" + uuids + "]";
+                    (new RuntimeException(msg)).printStackTrace();
+                    t.sendResponseHeaders(500, -1);
+                    os.write(msg.getBytes(UTF_8));
+                }
+                return;
+            }
+
+            String uuid = uuids.get(0);
+            // retrying
+            if (closedRequests.putIfAbsent(uuid, t.getRequestURI().toString()) == null) {
+                if (t.getExchangeVersion() == HttpClient.Version.HTTP_1_1) {
+                    // Throwing an exception here only causes a retry
+                    // with HTTP_1_1 - where it forces the server to close
+                    // the connection.
+                    // For HTTP/2 then throwing an IOE would cause the server
+                    // to close the stream, and throwing anything else would
+                    // cause it to close the connection, but neither would
+                    // cause the client to retry.
+                    // So we simply do not try to retry with HTTP/2 and just verify
+                    // we have received the expected cookie
+                    throw new IOException("Closing on first request");
+                }
+            }
+
+            // not retrying
+            readAllRequestData(t);
+            try (OutputStream os = t.getResponseBody()) {
+                List<String> cookie = t.getRequestHeaders().get("Cookie");
+
+                if (cookie == null || cookie.size() == 0) {
+                    String msg = "No cookie header present";
+                    (new RuntimeException(msg)).printStackTrace();
+                    t.sendResponseHeaders(500, -1);
+                    os.write(msg.getBytes(UTF_8));
+                } else if (!cookie.get(0).equals("CUSTOMER=ARTHUR_DENT")) {
+                    String msg = "Incorrect cookie header value:[" + cookie.get(0) + "]";
+                    (new RuntimeException(msg)).printStackTrace();
+                    t.sendResponseHeaders(500, -1);
+                    os.write(msg.getBytes(UTF_8));
+                } else if (cookie.size() > 1) {
+                    String msg = "Incorrect cookie header values:[" + cookie + "]";
+                    (new RuntimeException(msg)).printStackTrace();
+                    t.sendResponseHeaders(500, -1);
+                    os.write(msg.getBytes(UTF_8));
+                } else {
+                    assert cookie.get(0).equals("CUSTOMER=ARTHUR_DENT");
+                    byte[] bytes = MESSAGE.getBytes(UTF_8);
+                    for (String value : cookie) {
+                        t.getResponseHeaders().addHeader("X-Request-Cookie", value);
+                    }
+                    t.sendResponseHeaders(200, bytes.length);
+                    os.write(bytes);
+                }
+            }
+
+            closedRequests.remove(uuid);
+        }
+    }
+
+    static void readAllRequestData(HttpTestExchange t) throws IOException {
+        try (InputStream is = t.getRequestBody()) {
+            is.readAllBytes();
+        }
+    }
+}