From b2cff2e9163182b48812f8a01ffbf2eae5720a37 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Thu, 23 Jan 2014 19:01:18 +0100 Subject: [PATCH] Simplify settings and hide SPDY Variant construction. --- .../okhttp/internal/spdy/Http20Draft09.java | 12 ++-- .../squareup/okhttp/internal/spdy/Spdy3.java | 10 +-- .../okhttp/internal/spdy/SpdyConnection.java | 46 +++++++------ .../okhttp/internal/spdy/Variant.java | 21 +----- .../internal/spdy/Http20Draft09Test.java | 3 +- .../okhttp/internal/spdy/MockSpdyPeer.java | 4 +- .../internal/spdy/SpdyConnectionTest.java | 67 ++++++++++--------- 7 files changed, 71 insertions(+), 92 deletions(-) diff --git a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Http20Draft09.java b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Http20Draft09.java index faa83b2ec..defe6f5bd 100644 --- a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Http20Draft09.java +++ b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Http20Draft09.java @@ -38,11 +38,7 @@ public final class Http20Draft09 implements Variant { } // http://tools.ietf.org/html/draft-ietf-httpbis-http2-09#section-6.5 - @Override public Settings defaultOkHttpSettings(boolean client) { - return initialPeerSettings(client); - } - - @Override public Settings initialPeerSettings(boolean client) { + static Settings defaultSettings(boolean client) { Settings settings = new Settings(); settings.set(Settings.HEADER_TABLE_SIZE, 0, 4096); if (client) { // client specifies whether or not it accepts push. @@ -74,11 +70,11 @@ public final class Http20Draft09 implements Variant { static final int FLAG_PRIORITY = 0x8; static final int FLAG_ACK = 0x1; - @Override public FrameReader newReader(InputStream in, Settings peerSettings, boolean client) { - return new Reader(in, peerSettings.getHeaderTableSize(), client); + @Override public FrameReader newReader(InputStream in, boolean client) { + return new Reader(in, 4096, client); } - @Override public FrameWriter newWriter(OutputStream out, Settings ignored, boolean client) { + @Override public FrameWriter newWriter(OutputStream out, boolean client) { return new Writer(out, client); } diff --git a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Spdy3.java b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Spdy3.java index 184b901b3..e20c9d710 100644 --- a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Spdy3.java +++ b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Spdy3.java @@ -36,11 +36,7 @@ final class Spdy3 implements Variant { return Protocol.SPDY_3; } - @Override public Settings defaultOkHttpSettings(boolean client) { - return initialPeerSettings(client); // no difference in defaults. - } - - @Override public Settings initialPeerSettings(boolean client) { + static Settings defaultSettings(boolean client) { Settings settings = new Settings(); settings.set(Settings.INITIAL_WINDOW_SIZE, 0, 65535); return settings; @@ -104,11 +100,11 @@ final class Spdy3 implements Variant { } } - @Override public FrameReader newReader(InputStream in, Settings ignored, boolean client) { + @Override public FrameReader newReader(InputStream in, boolean client) { return new Reader(in, client); } - @Override public FrameWriter newWriter(OutputStream out, Settings ignored, boolean client) { + @Override public FrameWriter newWriter(OutputStream out, boolean client) { return new Writer(out, client); } diff --git a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java index c8cb75093..b5e143a1c 100644 --- a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java +++ b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java @@ -13,7 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package com.squareup.okhttp.internal.spdy; import com.squareup.okhttp.Protocol; @@ -61,7 +60,7 @@ public final class SpdyConnection implements Closeable { Util.threadFactory("OkHttp SpdyConnection", true)); /** The protocol variant, like {@link com.squareup.okhttp.internal.spdy.Spdy3}. */ - final Variant variant; + final Protocol protocol; /** True if this peer initiated the connection. */ final boolean client; @@ -93,27 +92,37 @@ public final class SpdyConnection implements Closeable { final ByteArrayPool bufferPool; private SpdyConnection(Builder builder) { - variant = builder.variant; + protocol = builder.protocol; client = builder.client; - okHttpSettings = variant.defaultOkHttpSettings(client); - // TODO: implement stream limit - // okHttpSettings.set(Settings.MAX_CONCURRENT_STREAMS, 0, max); - peerSettings = variant.initialPeerSettings(client); - bufferPool = new ByteArrayPool(peerSettings.getInitialWindowSize() * 8); handler = builder.handler; - frameReader = variant.newReader(builder.in, peerSettings, client); - frameWriter = variant.newWriter(builder.out, okHttpSettings, client); nextStreamId = builder.client ? 1 : 2; nextPingId = builder.client ? 1 : 2; - hostName = builder.hostName; + Variant variant; + if (protocol == Protocol.HTTP_2) { + okHttpSettings = Http20Draft09.defaultSettings(client); + variant = new Http20Draft09(); // connection-specific settings here! + } else if (protocol == Protocol.SPDY_3) { + okHttpSettings = Spdy3.defaultSettings(client); + variant = new Spdy3(); // connection-specific settings here! + } else { + throw new AssertionError(protocol); + } + + // TODO: implement stream limit + // okHttpSettings.set(Settings.MAX_CONCURRENT_STREAMS, 0, max); + peerSettings = okHttpSettings; + bufferPool = new ByteArrayPool(peerSettings.getInitialWindowSize() * 8); + frameReader = variant.newReader(builder.in, client); + frameWriter = variant.newWriter(builder.out, client); + new Thread(new Reader()).start(); // Not a daemon thread. } /** The protocol as selected using NPN or ALPN. */ public Protocol getProtocol() { - return variant.getProtocol(); + return protocol; } /** @@ -392,7 +401,7 @@ public final class SpdyConnection implements Closeable { private InputStream in; private OutputStream out; private IncomingStreamHandler handler = IncomingStreamHandler.REFUSE_INCOMING_STREAMS; - private Variant variant = Variant.SPDY3; + private Protocol protocol = Protocol.SPDY_3; private boolean client; public Builder(boolean client, Socket socket) throws IOException { @@ -428,14 +437,7 @@ public final class SpdyConnection implements Closeable { } public Builder protocol(Protocol protocol) { - // TODO: protocol == variant.getProtocol, so we could map this. - if (protocol == Protocol.HTTP_2) { - this.variant = Variant.HTTP_20_DRAFT_09; - } else if (protocol == Protocol.SPDY_3) { - this.variant = Variant.SPDY3; - } else { - throw new AssertionError(protocol); - } + this.protocol = protocol; return this; } @@ -550,7 +552,7 @@ public final class SpdyConnection implements Closeable { } else { peerSettings.merge(newSettings); } - if (SpdyConnection.this.variant.getProtocol() == Protocol.HTTP_2) { + if (getProtocol() == Protocol.HTTP_2) { ackSettingsLater(); } if (!streams.isEmpty()) { diff --git a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Variant.java b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Variant.java index f7367302c..784d02ef2 100644 --- a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Variant.java +++ b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Variant.java @@ -21,34 +21,17 @@ import java.io.OutputStream; /** A version and dialect of the framed socket protocol. */ interface Variant { - Variant SPDY3 = new Spdy3(); - Variant HTTP_20_DRAFT_09 = new Http20Draft09(); /** The protocol as selected using NPN or ALPN. */ Protocol getProtocol(); /** - * Default settings used for reading or writing frames to the peer. - * @param client true if these settings apply to writing requests, false if responses. - */ - Settings defaultOkHttpSettings(boolean client); - - /** - * Initial settings used for reading frames from the peer until we are sent - * a Settings frame. - * @param client true if these settings apply to reading responses, false if requests. - */ - Settings initialPeerSettings(boolean client); - - /** - * @param peerSettings potentially stale settings that reflect the remote peer. * @param client true if this is the HTTP client's reader, reading frames from a server. */ - FrameReader newReader(InputStream in, Settings peerSettings, boolean client); + FrameReader newReader(InputStream in, boolean client); /** - * @param okHttpSettings settings sent to the peer, such compression header table size. * @param client true if this is the HTTP client's writer, writing frames to a server. */ - FrameWriter newWriter(OutputStream out, Settings okHttpSettings, boolean client); + FrameWriter newWriter(OutputStream out, boolean client); } diff --git a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/Http20Draft09Test.java b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/Http20Draft09Test.java index 8f7c91714..a7f4cca60 100644 --- a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/Http20Draft09Test.java +++ b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/Http20Draft09Test.java @@ -456,8 +456,7 @@ public class Http20Draft09Test { } private Http20Draft09.Reader newReader(ByteArrayOutputStream out) { - return new Http20Draft09.Reader(new ByteArrayInputStream(out.toByteArray()), - Variant.HTTP_20_DRAFT_09.initialPeerSettings(false).getHeaderTableSize(), false); + return new Http20Draft09.Reader(new ByteArrayInputStream(out.toByteArray()), 4096, false); } private byte[] literalHeaders(List
sentHeaders) throws IOException { diff --git a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/MockSpdyPeer.java b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/MockSpdyPeer.java index 7ce058e82..e5335fa48 100644 --- a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/MockSpdyPeer.java +++ b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/MockSpdyPeer.java @@ -50,7 +50,7 @@ public final class MockSpdyPeer implements Closeable { public MockSpdyPeer(Variant variant, boolean client) { this.client = client; this.variant = variant; - this.frameWriter = variant.newWriter(bytesOut, variant.defaultOkHttpSettings(client), client); + this.frameWriter = variant.newWriter(bytesOut, client); } public void acceptFrame() { @@ -110,7 +110,7 @@ public final class MockSpdyPeer implements Closeable { socket = serverSocket.accept(); OutputStream out = socket.getOutputStream(); InputStream in = socket.getInputStream(); - FrameReader reader = variant.newReader(in, variant.initialPeerSettings(client), client); + FrameReader reader = variant.newReader(in, client); Iterator outFramesIterator = outFrames.iterator(); byte[] outBytes = bytesOut.toByteArray(); diff --git a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/SpdyConnectionTest.java b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/SpdyConnectionTest.java index d89d6648c..fbf656048 100644 --- a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/SpdyConnectionTest.java +++ b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/SpdyConnectionTest.java @@ -53,12 +53,15 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; public final class SpdyConnectionTest { + private static final Variant SPDY3 = new Spdy3(); + private static final Variant HTTP_20_DRAFT_09 = new Http20Draft09(); + private static final IncomingStreamHandler REJECT_INCOMING_STREAMS = new IncomingStreamHandler() { @Override public void receive(SpdyStream stream) throws IOException { throw new AssertionError(); } }; - private final MockSpdyPeer peer = new MockSpdyPeer(Variant.SPDY3, false); + private final MockSpdyPeer peer = new MockSpdyPeer(SPDY3, false); @After public void tearDown() throws Exception { peer.close(); @@ -189,7 +192,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.SPDY3); + SpdyConnection connection = connection(peer, SPDY3); connection.noop(); // verify the peer received what was expected @@ -204,7 +207,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - connection(peer, Variant.SPDY3); + connection(peer, SPDY3); // verify the peer received what was expected MockSpdyPeer.InFrame ping = peer.takeFrame(); @@ -215,7 +218,7 @@ public final class SpdyConnectionTest { } @Test public void serverPingsClientHttp2() throws Exception { - MockSpdyPeer peer = new MockSpdyPeer(Variant.HTTP_20_DRAFT_09, false); + MockSpdyPeer peer = new MockSpdyPeer(HTTP_20_DRAFT_09, false); // write the mocking script peer.sendFrame().ping(false, 2, 3); @@ -223,7 +226,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - connection(peer, Variant.HTTP_20_DRAFT_09); + connection(peer, HTTP_20_DRAFT_09); // verify the peer received what was expected MockSpdyPeer.InFrame ping = peer.takeFrame(); @@ -241,7 +244,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.SPDY3); + SpdyConnection connection = connection(peer, SPDY3); Ping ping = connection.ping(); assertTrue(ping.roundTripTime() > 0); assertTrue(ping.roundTripTime() < TimeUnit.SECONDS.toNanos(1)); @@ -255,7 +258,7 @@ public final class SpdyConnectionTest { } @Test public void clientPingsServerHttp2() throws Exception { - MockSpdyPeer peer = new MockSpdyPeer(Variant.HTTP_20_DRAFT_09, false); + MockSpdyPeer peer = new MockSpdyPeer(HTTP_20_DRAFT_09, false); // write the mocking script peer.acceptFrame(); // PING @@ -263,7 +266,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.HTTP_20_DRAFT_09); + SpdyConnection connection = connection(peer, HTTP_20_DRAFT_09); Ping ping = connection.ping(); assertTrue(ping.roundTripTime() > 0); assertTrue(ping.roundTripTime() < TimeUnit.SECONDS.toNanos(1)); @@ -286,7 +289,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - connection(peer, Variant.SPDY3); + connection(peer, SPDY3); // verify the peer received what was expected MockSpdyPeer.InFrame ping2 = peer.takeFrame(); @@ -297,7 +300,7 @@ public final class SpdyConnectionTest { @Test public void peerHttp2ServerZerosCompressionTable() throws Exception { boolean client = false; // Peer is server, so we are client. - Settings settings = Variant.HTTP_20_DRAFT_09.initialPeerSettings(client); + Settings settings = new Settings(); settings.set(Settings.HEADER_TABLE_SIZE, PERSIST_VALUE, 0); SpdyConnection connection = sendHttp2SettingsAndCheckForAck(client, settings); @@ -313,7 +316,7 @@ public final class SpdyConnectionTest { @Test public void peerHttp2ClientDisablesPush() throws Exception { boolean client = false; // Peer is client, so we are server. - Settings settings = Variant.HTTP_20_DRAFT_09.initialPeerSettings(client); + Settings settings = Http20Draft09.defaultSettings(client); settings.set(Settings.ENABLE_PUSH, 0, 0); // The peer client disables push. SpdyConnection connection = sendHttp2SettingsAndCheckForAck(client, settings); @@ -334,7 +337,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.SPDY3); + SpdyConnection connection = connection(peer, SPDY3); peer.takeFrame(); // Guarantees that the peer Settings frame has been processed. synchronized (connection) { @@ -359,7 +362,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.SPDY3); + SpdyConnection connection = connection(peer, SPDY3); peer.takeFrame(); // Guarantees that the Settings frame has been processed. synchronized (connection) { @@ -383,7 +386,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - connection(peer, Variant.SPDY3); + connection(peer, SPDY3); // verify the peer received what was expected MockSpdyPeer.InFrame rstStream = peer.takeFrame(); @@ -403,7 +406,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - connection(peer, Variant.SPDY3); + connection(peer, SPDY3); // verify the peer received what was expected MockSpdyPeer.InFrame rstStream = peer.takeFrame(); @@ -425,7 +428,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.SPDY3); + SpdyConnection connection = connection(peer, SPDY3); SpdyStream stream = connection.newStream(headerEntries("a", "android"), true, false); OutputStream out = stream.getOutputStream(); out.write("square".getBytes(UTF_8)); @@ -469,7 +472,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.SPDY3); + SpdyConnection connection = connection(peer, SPDY3); SpdyStream stream = connection.newStream(headerEntries("a", "android"), true, true); OutputStream out = stream.getOutputStream(); connection.ping().roundTripTime(); // Ensure that the RST_CANCEL has been received. @@ -509,7 +512,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.SPDY3); + SpdyConnection connection = connection(peer, SPDY3); SpdyStream stream = connection.newStream(headerEntries("a", "android"), false, true); InputStream in = stream.getInputStream(); OutputStream out = stream.getOutputStream(); @@ -552,7 +555,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.SPDY3); + SpdyConnection connection = connection(peer, SPDY3); SpdyStream stream = connection.newStream(headerEntries("a", "android"), true, true); InputStream in = stream.getInputStream(); OutputStream out = stream.getOutputStream(); @@ -594,7 +597,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.SPDY3); + SpdyConnection connection = connection(peer, SPDY3); SpdyStream stream = connection.newStream(headerEntries("a", "android"), false, true); InputStream in = stream.getInputStream(); assertStreamData("square", in); @@ -756,11 +759,11 @@ public final class SpdyConnectionTest { @Test public void receiveGoAway() throws Exception { - receiveGoAway(Variant.SPDY3); + receiveGoAway(SPDY3); } @Test public void receiveGoAwayHttp2() throws Exception { - receiveGoAway(Variant.HTTP_20_DRAFT_09); + receiveGoAway(HTTP_20_DRAFT_09); } private void receiveGoAway(Variant variant) throws Exception { @@ -999,7 +1002,7 @@ public final class SpdyConnectionTest { } @Test public void readSendsWindowUpdate() throws Exception { - readSendsWindowUpdate(Variant.SPDY3); + readSendsWindowUpdate(SPDY3); } /** @@ -1009,13 +1012,13 @@ public final class SpdyConnectionTest { * account max frame size per variant. */ @Test @Ignore public void readSendsWindowUpdateHttp2() throws Exception { - readSendsWindowUpdate(Variant.HTTP_20_DRAFT_09); + readSendsWindowUpdate(HTTP_20_DRAFT_09); } private void readSendsWindowUpdate(Variant variant) throws IOException, InterruptedException { MockSpdyPeer peer = new MockSpdyPeer(variant, false); - int windowUpdateThreshold = variant.initialPeerSettings(true).getInitialWindowSize() / 2; + int windowUpdateThreshold = 65535 / 2; // Write the mocking script. peer.acceptFrame(); // SYN_STREAM @@ -1054,7 +1057,7 @@ public final class SpdyConnectionTest { } @Test public void writeAwaitsWindowUpdate() throws Exception { - int windowSize = Variant.SPDY3.initialPeerSettings(true).getInitialWindowSize(); + int windowSize = 65535; // Write the mocking script. This accepts more data frames than necessary! peer.acceptFrame(); // SYN_STREAM @@ -1109,7 +1112,7 @@ public final class SpdyConnectionTest { * prevents us from overrunning the max frame size of SPDY/3 or HTTP/2. */ @Test public void spdyStreamOutputBufferSizeLimitsDataFrameLength() throws Exception { - MockSpdyPeer peer = new MockSpdyPeer(Variant.HTTP_20_DRAFT_09, false); + MockSpdyPeer peer = new MockSpdyPeer(HTTP_20_DRAFT_09, false); byte[] buff = new byte[SpdyStream.OUTPUT_BUFFER_SIZE * 2]; Arrays.fill(buff, (byte) '*'); @@ -1122,7 +1125,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.HTTP_20_DRAFT_09); + SpdyConnection connection = connection(peer, HTTP_20_DRAFT_09); SpdyStream stream = connection.newStream(headerEntries("b", "banana"), true, true); OutputStream out = stream.getOutputStream(); out.write(buff); @@ -1166,7 +1169,7 @@ public final class SpdyConnectionTest { // TODO: change this to only cancel when local settings disable push @Test public void pushPromiseStreamsAutomaticallyCancel() throws Exception { - MockSpdyPeer peer = new MockSpdyPeer(Variant.HTTP_20_DRAFT_09, false); + MockSpdyPeer peer = new MockSpdyPeer(HTTP_20_DRAFT_09, false); // write the mocking script peer.sendFrame().pushPromise(1, 2, Arrays.asList( @@ -1182,7 +1185,7 @@ public final class SpdyConnectionTest { peer.play(); // play it back - connection(peer, Variant.HTTP_20_DRAFT_09); + connection(peer, HTTP_20_DRAFT_09); // verify the peer received what was expected MockSpdyPeer.InFrame rstStream = peer.takeFrame(); @@ -1193,13 +1196,13 @@ public final class SpdyConnectionTest { private SpdyConnection sendHttp2SettingsAndCheckForAck(boolean client, Settings settings) throws IOException, InterruptedException { - MockSpdyPeer peer = new MockSpdyPeer(Variant.HTTP_20_DRAFT_09, client); + MockSpdyPeer peer = new MockSpdyPeer(HTTP_20_DRAFT_09, client); peer.sendFrame().settings(settings); peer.acceptFrame(); // ACK peer.play(); // play it back - SpdyConnection connection = connection(peer, Variant.HTTP_20_DRAFT_09); + SpdyConnection connection = connection(peer, HTTP_20_DRAFT_09); // verify the peer received the ACK MockSpdyPeer.InFrame pingFrame = peer.takeFrame();