8181430: HTTP/2 client might deadlock when receiving data during the initial handshake
Summary: CountDownLatch removed. Data produced during the handshake is instead buffered until the preface is sent.
Reviewed-by: michaelm, msheppar, prappo
--- a/jdk/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2Connection.java Thu Jun 08 11:24:46 2017 +0200
+++ b/jdk/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2Connection.java Thu Jun 08 12:24:13 2017 +0100
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2017, 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
@@ -111,47 +111,56 @@
*/
- // A small class that allows to control the state of
- // the connection preface. This is just a thin wrapper
- // over a CountDownLatch.
- private final class PrefaceController {
+ // A small class that allows to control frames with respect to the state of
+ // the connection preface. Any data received before the connection
+ // preface is sent will be buffered.
+ private final class FramesController {
volatile boolean prefaceSent;
- private final CountDownLatch latch = new CountDownLatch(1);
+ volatile List<ByteBufferReference> pending;
- // This method returns immediately if the preface is sent,
- // and blocks until the preface is sent if not.
- // In the common case this where the preface is already sent
- // this will cost not more than a volatile read.
- void waitUntilPrefaceSent() {
+ boolean processReceivedData(FramesDecoder decoder, ByteBufferReference buf)
+ throws IOException
+ {
+ // if preface is not sent, buffers data in the pending list
if (!prefaceSent) {
- try {
- // If the preface is not sent then await on the latch
- Log.logTrace("Waiting until connection preface is sent");
- latch.await();
- Log.logTrace("Preface sent: resuming reading");
- assert prefaceSent;
- } catch (InterruptedException e) {
- String msg = Utils.stackTrace(e);
- Log.logTrace(msg);
- shutdown(e);
+ synchronized (this) {
+ if (!prefaceSent) {
+ if (pending == null) pending = new ArrayList<>();
+ pending.add(buf);
+ return false;
+ }
}
}
+
+ // Preface is sent. Checks for pending data and flush it.
+ // We rely on this method being called from within the readlock,
+ // so we know that no other thread could execute this method
+ // concurrently while we're here.
+ // This ensures that later incoming buffers will not
+ // be processed before we have flushed the pending queue.
+ // No additional synchronization is therefore necessary here.
+ List<ByteBufferReference> pending = this.pending;
+ this.pending = null;
+ if (pending != null) {
+ // flush pending data
+ for (ByteBufferReference b : pending) {
+ decoder.decode(b);
+ }
+ }
+
+ // push the received buffer to the frames decoder.
+ decoder.decode(buf);
+ return true;
}
// Mark that the connection preface is sent
void markPrefaceSent() {
assert !prefaceSent;
- prefaceSent = true;
- // Release the latch. If asyncReceive was scheduled it will
- // be waiting for the release and will be woken up by this
- // call. If not, then the semaphore will no longer be used after
- // this.
- latch.countDown();
+ synchronized (this) {
+ prefaceSent = true;
+ }
}
- boolean isPrefaceSent() {
- return prefaceSent;
- }
}
volatile boolean closed;
@@ -176,7 +185,7 @@
* Each of this connection's Streams MUST use this controller.
*/
private final WindowController windowController = new WindowController();
- private final PrefaceController prefaceController = new PrefaceController();
+ private final FramesController framesController = new FramesController();
final WindowUpdateSender windowUpdater;
static final int DEFAULT_FRAME_SIZE = 16 * 1024;
@@ -409,11 +418,11 @@
// SettingsFrame sent by the server) before the connection
// preface is fully sent might result in the server
// sending a GOAWAY frame with 'invalid_preface'.
- prefaceController.waitUntilPrefaceSent();
synchronized (readlock) {
- assert prefaceController.isPrefaceSent();
try {
- framesDecoder.decode(buffer);
+ // the readlock ensures that the order of incoming buffers
+ // is preserved.
+ framesController.processReceivedData(framesDecoder, buffer);
} catch (Throwable e) {
String msg = Utils.stackTrace(e);
Log.logTrace(msg);
@@ -646,7 +655,8 @@
Log.logFrames(sf, "OUT");
// send preface bytes and SettingsFrame together
connection.write(ref.get());
-
+ // mark preface sent.
+ framesController.markPrefaceSent();
Log.logTrace("PREFACE_BYTES sent");
Log.logTrace("Settings Frame sent");
@@ -654,8 +664,10 @@
// minus the initial 64 K specified in protocol
final int len = client2.client().getReceiveBufferSize() - (64 * 1024 - 1);
windowUpdater.sendWindowUpdate(len);
+ // there will be an ACK to the windows update - which should
+ // cause any pending data stored before the preface was sent to be
+ // flushed (see PrefaceController).
Log.logTrace("finished sending connection preface");
- prefaceController.markPrefaceSent();
}
/**