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 546109024..1cab01172 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 @@ public final class FramedServer extends FramedConnection.Listener { .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 1e4db28d4..8ff4708d7 100644 --- a/mockwebserver/src/main/java/okhttp3/mockwebserver/MockWebServer.java +++ b/mockwebserver/src/main/java/okhttp3/mockwebserver/MockWebServer.java @@ -449,6 +449,7 @@ public final class MockWebServer implements TestRule { .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 fb2e3509a..b2a583857 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 @@ public final class Http2ConnectionTest { // 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 @@ public final class Http2ConnectionTest { // 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 @@ public final class Http2ConnectionTest { 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 @@ public final class Http2ConnectionTest { .pushObserver(IGNORE) .protocol(HTTP_2.getProtocol()) .build(); + connection.start(false); socket.shutdownOutput(); try { connection.newStream(headerEntries("a", longString), false, true); @@ -488,7 +494,9 @@ public final class Http2ConnectionTest { } 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 58cdc04bd..96a6fb428 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 @@ public final class Spdy3ConnectionTest { .socket(socket) .protocol(SPDY3.getProtocol()) .build(); + connection.start(false); socket.shutdownOutput(); try { connection.newStream(headerEntries("a", longString), false, true); @@ -1360,7 +1367,9 @@ public final class Spdy3ConnectionTest { } 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 0cfcba72f..4a6a8e7d0 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 @@ public final class FramedConnection implements Closeable { 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 @@ public final class FramedConnection implements Closeable { } /** - * 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 ed8019e2f..ffe889814 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 @@ public final class RealConnection extends FramedConnection.Listener implements C .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();