Merge pull request square#2473 from square/jwilson_0410_connection_pr…

…eface

Don't start the reader thread until after the connection preface.

mockwebserver/src/main/java/okhttp3/internal/framed/FramedServer.java

@@ -70,7 +70,7 @@ private void run() throws Exception {
             .protocol(protocol)
             .listener(this)
             .build();
-        framedConnection.sendConnectionPreface();
+        framedConnection.start();
       } catch (IOException e) {
         logger.log(Level.INFO, "FramedServer connection failure: " + e);
         Util.closeQuietly(socket);

mockwebserver/src/main/java/okhttp3/mockwebserver/MockWebServer.java

@@ -449,6 +449,7 @@ public void processConnection() throws Exception {
               .protocol(protocol)
               .listener(framedSocketListener)
               .build();
+          framedConnection.start();
           openFramedConnections.add(framedConnection);
           openClientSockets.remove(socket);
           return;

okhttp-tests/src/test/java/okhttp3/internal/framed/Http2ConnectionTest.java

@@ -376,7 +376,9 @@ private Buffer data(int byteCount) {
 
     // play it back
     FramedConnection connection = connectionBuilder(peer, HTTP_2)
-        .pushObserver(observer).build();
+        .pushObserver(observer)
+        .build();
+    connection.start(false);
     FramedStream client = connection.newStream(headerEntries("b", "banana"), false, true);
     assertEquals(-1, client.getSource().read(new Buffer(), 1));
 
@@ -399,6 +401,7 @@ private Buffer data(int byteCount) {
 
     // play it back
     FramedConnection connection = connectionBuilder(peer, HTTP_2).build();
+    connection.start(false);
     connection.newStream(headerEntries("b", "banana"), false, true);
 
     // verify the peer received what was expected
@@ -423,8 +426,10 @@ private Buffer data(int byteCount) {
     peer.play();
 
     // play it back
-    connectionBuilder(peer, HTTP_2)
-        .pushObserver(PushObserver.CANCEL).build();
+    FramedConnection connection = connectionBuilder(peer, HTTP_2)
+        .pushObserver(PushObserver.CANCEL)
+        .build();
+    connection.start(false);
 
     // verify the peer received what was expected
     MockSpdyPeer.InFrame rstStream = peer.takeFrame();
@@ -452,6 +457,7 @@ private Buffer data(int byteCount) {
         .pushObserver(IGNORE)
         .protocol(HTTP_2.getProtocol())
         .build();
+    connection.start(false);
     socket.shutdownOutput();
     try {
       connection.newStream(headerEntries("a", longString), false, true);
@@ -488,7 +494,9 @@ private FramedConnection sendHttp2SettingsAndCheckForAck(boolean client, Setting
   }
 
   private FramedConnection connection(MockSpdyPeer peer, Variant variant) throws IOException {
-    return connectionBuilder(peer, variant).build();
+    FramedConnection connection = connectionBuilder(peer, variant).build();
+    connection.start(false);
+    return connection;
   }
 
   private FramedConnection.Builder connectionBuilder(MockSpdyPeer peer, Variant variant)

okhttp-tests/src/test/java/okhttp3/internal/framed/Spdy3ConnectionTest.java

@@ -162,10 +162,11 @@ public final class Spdy3ConnectionTest {
         stream.reply(headerEntries("b", "banana"), true);
       }
     };
-    new FramedConnection.Builder(true)
+    FramedConnection connection = new FramedConnection.Builder(true)
         .socket(peer.openSocket())
         .listener(handler)
         .build();
+    connection.start(false);
 
     // verify the peer received what was expected
     MockSpdyPeer.InFrame reply = peer.takeFrame();
@@ -192,7 +193,10 @@ public final class Spdy3ConnectionTest {
       }
     };
 
-    connectionBuilder(peer, SPDY3).listener(listener).build();
+    FramedConnection connection = connectionBuilder(peer, SPDY3)
+        .listener(listener)
+        .build();
+    connection.start(false);
 
     // verify the peer received what was expected
     MockSpdyPeer.InFrame reply = peer.takeFrame();
@@ -282,6 +286,7 @@ public final class Spdy3ConnectionTest {
     FramedConnection connection = connectionBuilder(peer, SPDY3)
         .listener(listener)
         .build();
+    connection.start(false);
 
     peer.takeFrame(); // Guarantees that the peer Settings frame has been processed.
     synchronized (connection) {
@@ -639,10 +644,11 @@ public final class Spdy3ConnectionTest {
         stream.reply(headerEntries("c", "cola"), true);
       }
     };
-    new FramedConnection.Builder(true)
+    FramedConnection connection = new FramedConnection.Builder(true)
         .socket(peer.openSocket())
         .listener(listener)
         .build();
+    connection.start(false);
 
     // verify the peer received what was expected
     MockSpdyPeer.InFrame reply = peer.takeFrame();
@@ -1340,6 +1346,7 @@ private void headerBlockHasTrailingCompressedBytes(String frame, int length) thr
         .socket(socket)
         .protocol(SPDY3.getProtocol())
         .build();
+    connection.start(false);
     socket.shutdownOutput();
     try {
       connection.newStream(headerEntries("a", longString), false, true);
@@ -1360,7 +1367,9 @@ private byte[] randomBytes(int length) {
   }
 
   private FramedConnection connection(MockSpdyPeer peer, Variant variant) throws IOException {
-    return connectionBuilder(peer, variant).build();
+    FramedConnection connection = connectionBuilder(peer, variant).build();
+    connection.start(false);
+    return connection;
   }
 
   private FramedConnection.Builder connectionBuilder(MockSpdyPeer peer, Variant variant)

okhttp/src/main/java/okhttp3/internal/framed/FramedConnection.java

@@ -170,7 +170,6 @@ private FramedConnection(Builder builder) throws IOException {
     frameWriter = variant.newWriter(builder.sink, client);
 
     readerRunnable = new Reader(variant.newReader(builder.source, client));
-    new Thread(readerRunnable).start(); // Not a daemon thread.
   }
 
   /** The protocol as selected using ALPN. */
@@ -501,16 +500,27 @@ private void close(ErrorCode connectionCode, ErrorCode streamCode) throws IOExce
   }
 
   /**
-   * Sends a connection header if the current variant requires it. This should be called after
-   * {@link Builder#build} for all new connections.
+   * Sends any initial frames and starts reading frames from the remote peer. This should be called
+   * after {@link Builder#build} for all new connections.
    */
-  public void sendConnectionPreface() throws IOException {
-    frameWriter.connectionPreface();
-    frameWriter.settings(okHttpSettings);
-    int windowSize = okHttpSettings.getInitialWindowSize(Settings.DEFAULT_INITIAL_WINDOW_SIZE);
-    if (windowSize != Settings.DEFAULT_INITIAL_WINDOW_SIZE) {
-      frameWriter.windowUpdate(0, windowSize - Settings.DEFAULT_INITIAL_WINDOW_SIZE);
+  public void start() throws IOException {
+    start(true);
+  }
+
+  /**
+   * @param sendConnectionPreface true to send connection preface frames. This should always be true
+   *     except for in tests that don't check for a connection preface.
+   */
+  void start(boolean sendConnectionPreface) throws IOException {
+    if (sendConnectionPreface) {
+      frameWriter.connectionPreface();
+      frameWriter.settings(okHttpSettings);
+      int windowSize = okHttpSettings.getInitialWindowSize(Settings.DEFAULT_INITIAL_WINDOW_SIZE);
+      if (windowSize != Settings.DEFAULT_INITIAL_WINDOW_SIZE) {
+        frameWriter.windowUpdate(0, windowSize - Settings.DEFAULT_INITIAL_WINDOW_SIZE);
+      }
     }
+    new Thread(readerRunnable).start(); // Not a daemon thread.
   }
 
   /** Merges {@code settings} into this peer's settings and sends them to the remote peer. */

okhttp/src/main/java/okhttp3/internal/io/RealConnection.java

@@ -207,7 +207,7 @@ private void establishProtocol(int readTimeout, int writeTimeout,
           .protocol(protocol)
           .listener(this)
           .build();
-      framedConnection.sendConnectionPreface();
+      framedConnection.start();
 
       // Only assign the framed connection once the preface has been sent successfully.
       this.allocationLimit = framedConnection.maxConcurrentStreams();