8181430: HTTP/2 client might deadlock when receiving data during the initial handshake

CountDownLatch removed. Data produced during the handshake is instead buffered until the preface is sent.

Reviewed-by: michaelm, msheppar, prappo

jdk/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/Http2Connection.java

@@ -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 @@ class Http2Connection  {
      */
 
 
-    // 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 @@ class Http2Connection  {
      * 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 @@ class Http2Connection  {
         // 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 @@ class Http2Connection  {
         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 @@ class Http2Connection  {
         // 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();
     }
 
     /**