Merge pull request #479 from adriancole/hide-settings

Simplify settings and hide SPDY Variant construction

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);
   }
 

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);
   }
 

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()) {

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);
 }

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<Header> sentHeaders) throws IOException {

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<OutFrame> outFramesIterator = outFrames.iterator();
     byte[] outBytes = bytesOut.toByteArray();

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();