http-client-branch: Fixed h2c connection reuse problem (connections not being reused/cached)
--- 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> {