http-client-branch: Fixed h2c connection reuse problem (connections not being reused/cached) http-client-branch
authormichaelm
Mon, 11 Dec 2017 16:43:11 +0000
branchhttp-client-branch
changeset 55981 907bddce488c
parent 55978 65bd0b92abec
child 55982 b6ff245c0db6
http-client-branch: Fixed h2c connection reuse problem (connections not being reused/cached)
src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Exchange.java
src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2ClientImpl.java
src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2Connection.java
src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/PlainHttpConnection.java
src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Stream.java
--- a/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Exchange.java	Fri Dec 08 12:35:49 2017 +0300
+++ b/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Exchange.java	Mon Dec 11 16:43:11 2017 +0000
@@ -373,17 +373,22 @@
                                                  client.client2(),
                                                  this, e::drainLeftOverBytes)
                         .thenCompose((Http2Connection c) -> {
-                            c.putConnection();
+                            boolean cached = c.offerConnection();
                             Stream<T> s = c.getStream(1);
+
                             if (s == null) {
                                 // s can be null if an exception occurred
                                 // asynchronously while sending the preface.
                                 Throwable t = c.getRecordedCause();
                                 if (t != null) {
+                                    if (!cached)
+                                        c.close();
                                     return MinimalFuture.failedFuture(
                                             new IOException("Can't get stream 1: " + t, t));
                                 }
                             }
+                            if (!cached)
+                                s.closeConnectionOnCompletion();
                             exchImpl.released();
                             Throwable t;
                             // There's a race condition window where an external
--- a/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2ClientImpl.java	Fri Dec 08 12:35:49 2017 +0300
+++ b/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2ClientImpl.java	Mon Dec 11 16:43:11 2017 +0000
@@ -93,7 +93,7 @@
      *
      * If the request is secure (https) then we open the connection here.
      * If not, then the more complicated upgrade from 1.1 to 2 happens (not here)
-     * In latter case, when the Http2Connection is connected, putConnection() must
+     * In latter case, when the Http2Connection is connected, offerConnection() must
      * be called to store it.
      */
     CompletableFuture<Http2Connection> getConnectionFor(HttpRequestImpl req) {
@@ -130,7 +130,7 @@
                         debug.log(Level.DEBUG, "Opening completed: %s", key);
                         opening.remove(key);
                         if (t == null && conn != null)
-                            putConnection(conn);
+                            offerConnection(conn);
                         final Throwable cause = Utils.getCompletionCause(t);
                         if (waiters == null) {
                             debug.log(Level.DEBUG, "no dependent to wake up");
@@ -152,11 +152,16 @@
     }
 
     /*
-     * TODO: If there isn't a connection to the same destination, then
-     * store it. If there is already a connection, then close it
+     * Cache the given connection, if no connection to the same
+     * destination exists. If one exists, then we let the initial stream
+     * complete but allow it to close itself upon completion.
+     * This situation should not arise with https because the request
+     * has not been sent as part of the initial alpn negotiation
      */
-    void putConnection(Http2Connection c) {
-        connections.put(c.key(), c);
+    boolean offerConnection(Http2Connection c) {
+        String key = c.key();
+        Http2Connection c1 = connections.putIfAbsent(key, c);
+        return c1 == null;
     }
 
     void deleteConnection(Http2Connection c) {
--- a/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2Connection.java	Fri Dec 08 12:35:49 2017 +0300
+++ b/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2Connection.java	Mon Dec 11 16:43:11 2017 +0000
@@ -427,6 +427,10 @@
     // P indicates proxy
     // Eg: "S:H:foo.com:80"
     static String keyString(boolean secure, boolean proxy, String host, int port) {
+        if (secure && port == -1)
+            port = 443;
+        else if (!secure && port == -1)
+            port = 80;
         return (secure ? "S:" : "C:") + (proxy ? "P:" : "H:") + host + ":" + port;
     }
 
@@ -434,8 +438,8 @@
         return this.key;
     }
 
-    void putConnection() {
-        client2.putConnection(this);
+    boolean offerConnection() {
+        return client2.offerConnection(this);
     }
 
     private HttpPublisher publisher() {
--- a/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/PlainHttpConnection.java	Fri Dec 08 12:35:49 2017 +0300
+++ b/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/PlainHttpConnection.java	Mon Dec 11 16:43:11 2017 +0000
@@ -84,7 +84,7 @@
                 boolean finished = chan.finishConnect();
                 assert finished : "Expected channel to be connected";
                 debug.log(Level.DEBUG,
-                          "ConnectEvent: connect finished: %s", finished);
+                          "ConnectEvent: connect finished: %s Local addr: %s", finished, chan.getLocalAddress());
                 connected = true;
                 // complete async since the event runs on the SelectorManager thread
                 cf.completeAsync(() -> null, client().theExecutor());
--- a/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Stream.java	Fri Dec 08 12:35:49 2017 +0300
+++ b/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Stream.java	Mon Dec 11 16:43:11 2017 +0000
@@ -109,6 +109,7 @@
     final Http2Connection connection;
     final HttpRequestImpl request;
     final DecodingCallback rspHeadersConsumer;
+    volatile boolean closeConnectionOnCompletion;
     HttpHeadersImpl responseHeaders;
     final HttpHeadersImpl requestPseudoHeaders;
     volatile HttpResponse.BodySubscriber<T> responseSubscriber;
@@ -196,6 +197,17 @@
         }
     }
 
+    // call this anywhere the stream is terminated
+    void checkConnectionClosure() {
+        if (closeConnectionOnCompletion) {
+            connection.close();
+        }
+    }
+
+    void closeConnectionOnCompletion() {
+        closeConnectionOnCompletion = true;
+    }
+
     // Callback invoked after the Response BodySubscriber has consumed the
     // buffers contained in a DataFrame.
     // Returns true if END_STREAM is reached, false otherwise.
@@ -936,6 +948,7 @@
         } catch (IOException ex) {
             Log.logError(ex);
         }
+        checkConnectionClosure();
     }
 
     // This method doesn't send any frame
@@ -948,6 +961,7 @@
         Log.logTrace("Closing stream {0}", streamid);
         connection.closeStream(streamid);
         Log.logTrace("Stream {0} closed", streamid);
+        checkConnectionClosure();
     }
 
     static class PushedStream<U,T> extends Stream<T> {