From 8996934748a48fe04f7dde56069042dfecb64c91 Mon Sep 17 00:00:00 2001 From: jwilson Date: Sun, 10 Apr 2016 12:51:38 -0400 Subject: [PATCH] Don't start the reader thread until after the connection preface. This is slightly more work than ideal because our tests don't bother with the connection preface. That makes the tests both simpler, and further from reality. The workaround is a package-private method to keep the tests working as they are currently. Closes https://github.com/square/okhttp/issues/2469 --- .../okhttp3/internal/framed/FramedServer.java | 2 +- .../okhttp3/mockwebserver/MockWebServer.java | 1 + .../internal/framed/Http2ConnectionTest.java | 16 ++++++++--- .../internal/framed/Spdy3ConnectionTest.java | 17 ++++++++--- .../internal/framed/FramedConnection.java | 28 +++++++++++++------ .../okhttp3/internal/io/RealConnection.java | 2 +- 6 files changed, 47 insertions(+), 19 deletions(-) diff --git a/mockwebserver/src/main/java/okhttp3/internal/framed/FramedServer.java b/mockwebserver/src/main/java/okhttp3/internal/framed/FramedServer.java index 546109024627..1cab0117219b 100644 --- a/mockwebserver/src/main/java/okhttp3/internal/framed/FramedServer.java +++ b/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); diff --git a/mockwebserver/src/main/java/okhttp3/mockwebserver/MockWebServer.java b/mockwebserver/src/main/java/okhttp3/mockwebserver/MockWebServer.java index 1e4db28d4178..8ff4708d723a 100644 --- a/mockwebserver/src/main/java/okhttp3/mockwebserver/MockWebServer.java +++ b/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; diff --git a/okhttp-tests/src/test/java/okhttp3/internal/framed/Http2ConnectionTest.java b/okhttp-tests/src/test/java/okhttp3/internal/framed/Http2ConnectionTest.java index fb2e3509aebc..b2a583857d53 100644 --- a/okhttp-tests/src/test/java/okhttp3/internal/framed/Http2ConnectionTest.java +++ b/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) diff --git a/okhttp-tests/src/test/java/okhttp3/internal/framed/Spdy3ConnectionTest.java b/okhttp-tests/src/test/java/okhttp3/internal/framed/Spdy3ConnectionTest.java index 58cdc04bde63..96a6fb428899 100644 --- a/okhttp-tests/src/test/java/okhttp3/internal/framed/Spdy3ConnectionTest.java +++ b/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) diff --git a/okhttp/src/main/java/okhttp3/internal/framed/FramedConnection.java b/okhttp/src/main/java/okhttp3/internal/framed/FramedConnection.java index 0cfcba72fa88..4a6a8e7d0943 100644 --- a/okhttp/src/main/java/okhttp3/internal/framed/FramedConnection.java +++ b/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. */ diff --git a/okhttp/src/main/java/okhttp3/internal/io/RealConnection.java b/okhttp/src/main/java/okhttp3/internal/io/RealConnection.java index ed8019e2f61b..ffe889814c5a 100644 --- a/okhttp/src/main/java/okhttp3/internal/io/RealConnection.java +++ b/okhttp/src/main/java/okhttp3/internal/io/RealConnection.java @@ -159,7 +159,7 @@ private void connectSocket(int connectTimeout, 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();