address comments from issue #400

okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/ErrorCode.java

@@ -1,5 +1,7 @@
 package com.squareup.okhttp.internal.spdy;
 
+// TODO: revisit for http/2 draft 9
+// http://tools.ietf.org/html/draft-ietf-httpbis-http2-09#section-7
 public enum ErrorCode {
   /** Not an error! For SPDY stream resets, prefer null over NO_ERROR. */
   NO_ERROR(0, -1, 0),

okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/HpackDraft05.java

@@ -7,7 +7,6 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.BitSet;
 import java.util.List;
-import java.util.ListIterator;
 
 /**
  * Read and write HPACK v05.
@@ -45,7 +44,6 @@ final class HpackDraft05 {
     }
   }
 
-  static final int PREFIX_5_BITS = 0x1f;
   static final int PREFIX_6_BITS = 0x3f;
   static final int PREFIX_7_BITS = 0x7f;
   static final int PREFIX_8_BITS = 0xff;
@@ -133,16 +131,6 @@ final class HpackDraft05 {
       this.in = in;
     }
 
-    // Visible for testing.
-    void reset() {
-      bytesLeft = 0;
-      headerTableSize = 0;
-      maxHeaderTableSize = 4096;
-      staticReferenceSet.clear();
-      headerTable.clear();
-      emittedHeaders.clear();
-    }
-
     /**
      * Read {@code byteCount} bytes of headers from the source stream into the
      * set of emitted headers.
@@ -157,7 +145,7 @@ final class HpackDraft05 {
         if ((b & 0x80) != 0) {
           int index = readInt(b, PREFIX_7_BITS);
           if (index == 0) {
-            emptyReferenceSet();
+            clearReferenceSet();
           } else {
             readIndexedHeader(index - 1);
           }
@@ -172,12 +160,13 @@ final class HpackDraft05 {
           int index = readInt(b, PREFIX_6_BITS);
           readLiteralHeaderWithIncrementalIndexingIndexedName(index - 1);
         } else {
-          throw new AssertionError();
+          // TODO: we should throw something that we can coerce to a PROTOCOL_ERROR
+          throw new AssertionError("unhandled byte: " + Integer.toBinaryString(b));
         }
       }
     }
 
-    private void emptyReferenceSet() {
+    private void clearReferenceSet() {
       staticReferenceSet.clear();
       for (HeaderEntry entry : headerTable) {
         entry.referenced = false;
@@ -189,9 +178,8 @@ final class HpackDraft05 {
           i = staticReferenceSet.nextSetBit(i + 1)) {
         STATIC_HEADER_TABLE.get(i).addTo(emittedHeaders);
       }
-      for (ListIterator<HeaderEntry> li = headerTable.listIterator(headerTable.size());
-          li.hasPrevious(); ) {
-        li.previous().addTo(emittedHeaders);
+      for (int i = headerTable.size() - 1; i != -1; i--) {
+        headerTable.get(i).addTo(emittedHeaders);
       }
     }
 
@@ -217,6 +205,9 @@ final class HpackDraft05 {
         HeaderEntry existing = headerTable.get(index);
         existing.referenced = true;
         insertIntoHeaderTable(index, existing);
+      } else {
+        // TODO: we should throw something that we can coerce to a PROTOCOL_ERROR
+        throw new AssertionError("invalid index " + index);
       }
     }
 

okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Http20Draft09.java

@@ -25,7 +25,6 @@ import java.io.OutputStream;
 import java.io.UnsupportedEncodingException;
 import java.util.Arrays;
 import java.util.List;
-import java.util.Locale;
 
 /**
  * Read and write http/2 v09 frames.
@@ -159,13 +158,12 @@ public final class Http20Draft09 implements Variant {
 
         if ((flags & FLAG_END_HEADERS) != 0) {
           hpackReader.emitReferenceSet();
-          List<String> namesAndValues = hpackReader.getAndReset();
-          // TODO: throw malformed if any present:
-          // Connection, Keep-Alive, Proxy-Connection, TE, Transfer-Encoding, Encoding, Upgrade.
+          // not filtering out illegal headers on read.
+          List<String> nameValueBlock = hpackReader.getAndReset();
           // TODO: Concat multi-value headers with 0x0, except COOKIE, which uses 0x3B, 0x20.
           // http://tools.ietf.org/html/draft-ietf-httpbis-http2-09#section-8.1.3
           int priority = -1; // TODO: priority
-          handler.headers(false, inFinished, streamId, -1, priority, namesAndValues,
+          handler.headers(false, inFinished, streamId, -1, priority, nameValueBlock,
               HeadersMode.HTTP_20_HEADERS);
           return;
         }
@@ -325,13 +323,19 @@ public final class Http20Draft09 implements Variant {
         List<String> nameValueBlock) throws IOException {
       hpackBuffer.reset();
       for (int i = 0, size = nameValueBlock.size(); i < size; i += 2) {
-        String headerName = nameValueBlock.get(i).toLowerCase(Locale.US);
+        String name = nameValueBlock.get(i);
         // our SpdyTransport.writeNameValueBlock hard-codes :host
-        if (":host".equals(headerName)) headerName = ":authority";
-        nameValueBlock.set(i, headerName);
+        // TODO: is :authority literally the same value as :host?
+        // https://github.com/http2/http2-spec/issues/334
+        if (":host".equals(name)) {
+          nameValueBlock.set(i, ":authority");
+        } else if (shouldDropHeader(name)) {
+          //TODO: Avoid creating headers like these.
+          nameValueBlock.remove(i);
+          nameValueBlock.remove(i);
+          i -= 2;
+        }
       }
-      // TODO: throw malformed if any present:
-      // Connection, Keep-Alive, Proxy-Connection, TE, Transfer-Encoding, Encoding, Upgrade.
       hpackWriter.writeHeaders(nameValueBlock);
       int type = TYPE_HEADERS;
       // TODO: implement CONTINUATION
@@ -354,7 +358,7 @@ public final class Http20Draft09 implements Variant {
       int length = 4;
       out.writeInt((length & 0x3fff) << 16 | (type & 0xff) << 8 | (flags & 0xff));
       out.writeInt(streamId & 0x7fffffff);
-      out.writeInt(errorCode.spdyRstCode);
+      out.writeInt(errorCode.httpCode);
       out.flush();
     }
 
@@ -367,6 +371,7 @@ public final class Http20Draft09 implements Variant {
       int type = TYPE_DATA;
       int flags = 0;
       if (outFinished) flags |= FLAG_END_STREAM;
+      // TODO: Implement looping strategy.
       checkFrameSize(byteCount);
       out.writeInt((byteCount & 0x3fff) << 16 | (type & 0xff) << 8 | (flags & 0xff));
       out.writeInt(streamId & 0x7fffffff);
@@ -418,4 +423,19 @@ public final class Http20Draft09 implements Variant {
   private static IOException ioException(String message, Object... args) throws IOException {
     throw new IOException(String.format(message, args));
   }
+
+  /**
+   * Leniently drop as opposed to throwing malformed.
+   * http://tools.ietf.org/html/draft-ietf-httpbis-http2-09#section-8.1.3
+   */
+  private static boolean shouldDropHeader(String name) {
+    return name.equals("connection")
+        || name.equals("host") // host is not supported in http/2
+        || name.equals("keep-alive")
+        || name.equals("proxy-connection")
+        || name.equals("te")
+        || name.equals("transfer-encoding")
+        || name.equals("encoding")
+        || name.equals("upgrade");
+  }
 }

okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Settings.java

@@ -40,7 +40,7 @@ final class Settings {
   static final int HEADER_TABLE_SIZE = 1;
   /** spdy/3: Sender's estimate of max outgoing kbps. */
   static final int DOWNLOAD_BANDWIDTH = 2;
-  /** http/2: An endpoint must not send a PUSH_PROMISE frame this is 0. */
+  /** http/2: An endpoint must not send a PUSH_PROMISE frame when this is 0. */
   static final int ENABLE_PUSH = 2;
   /** spdy/3: Sender's estimate of millis between sending a request and receiving a response. */
   static final int ROUND_TRIP_TIME = 3;

okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/HpackDraft05Test.java

@@ -22,20 +22,18 @@ import java.io.DataOutputStream;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.List;
-import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
 
 public class HpackDraft05Test {
 
   private final MutableByteArrayInputStream bytesIn = new MutableByteArrayInputStream();
-  private final HpackDraft05.Reader hpackReader = new HpackDraft05.Reader(new DataInputStream(bytesIn));
+  private HpackDraft05.Reader hpackReader;
 
-  @After public void resetReader(){
-    hpackReader.reset();
+  @Before public void resetReader() {
+    hpackReader = new HpackDraft05.Reader(new DataInputStream(bytesIn));
   }
 
   /**
@@ -47,8 +45,9 @@ public class HpackDraft05Test {
     Arrays.fill(tooLarge, 'a');
     final List<String> sentHeaders = Arrays.asList("foo", new String(tooLarge));
 
-    bytesIn.set(literalHeaders(sentHeaders));
-    hpackReader.readHeaders(bytesIn.available());
+    ByteArrayOutputStream out = literalHeaders(sentHeaders);
+    bytesIn.set(out.toByteArray());
+    hpackReader.readHeaders(out.size());
     hpackReader.emitReferenceSet();
 
     assertEquals(0, hpackReader.headerTable.size());
@@ -70,16 +69,14 @@ public class HpackDraft05Test {
     out.write("custom-header".getBytes(), 0, 13);
 
     bytesIn.set(out.toByteArray());
-    hpackReader.readHeaders(bytesIn.available());
+    hpackReader.readHeaders(out.size());
     hpackReader.emitReferenceSet();
 
     assertEquals(1, hpackReader.headerTable.size());
+    assertEquals(55, hpackReader.headerTableSize);
 
     HpackDraft05.HeaderEntry entry = hpackReader.headerTable.get(0);
-    assertEquals("custom-key", entry.name);
-    assertEquals("custom-header", entry.value);
-    assertEquals(55, entry.size);
-    assertEquals(entry.size, hpackReader.headerTableSize);
+    checkEntry(entry, "custom-key", "custom-header", 55, true);
 
     assertEquals(Arrays.asList("custom-key", "custom-header"), hpackReader.getAndReset());
   }
@@ -96,7 +93,7 @@ public class HpackDraft05Test {
     out.write("/sample/path".getBytes(), 0, 12);
 
     bytesIn.set(out.toByteArray());
-    hpackReader.readHeaders(bytesIn.available());
+    hpackReader.readHeaders(out.size());
     hpackReader.emitReferenceSet();
 
     assertEquals(0, hpackReader.headerTable.size());
@@ -114,16 +111,14 @@ public class HpackDraft05Test {
                      // idx = 2 -> :method: GET
 
     bytesIn.set(out.toByteArray());
-    hpackReader.readHeaders(bytesIn.available());
+    hpackReader.readHeaders(out.size());
     hpackReader.emitReferenceSet();
 
     assertEquals(1, hpackReader.headerTable.size());
+    assertEquals(42, hpackReader.headerTableSize);
 
     HpackDraft05.HeaderEntry entry = hpackReader.headerTable.get(0);
-    assertEquals(":method", entry.name);
-    assertEquals("GET", entry.value);
-    assertEquals(42, entry.size);
-    assertEquals(entry.size, hpackReader.headerTableSize);
+    checkEntry(entry, ":method", "GET", 42, true);
 
     assertEquals(Arrays.asList(":method", "GET"), hpackReader.getAndReset());
   }
@@ -139,7 +134,7 @@ public class HpackDraft05Test {
 
     bytesIn.set(out.toByteArray());
     hpackReader.maxHeaderTableSize = 0; // SETTINGS_HEADER_TABLE_SIZE == 0
-    hpackReader.readHeaders(bytesIn.available());
+    hpackReader.readHeaders(out.size());
     hpackReader.emitReferenceSet();
 
     // Not buffered in header table.
@@ -152,23 +147,26 @@ public class HpackDraft05Test {
    * http://tools.ietf.org/html/draft-ietf-httpbis-header-compression-05#appendix-E.2
    */
   @Test public void decodeRequestExamplesWithoutHuffman() throws IOException {
-    bytesIn.set(firstRequestWithoutHuffman());
-    hpackReader.readHeaders(bytesIn.available());
+    ByteArrayOutputStream out = firstRequestWithoutHuffman();
+    bytesIn.set(out.toByteArray());
+    hpackReader.readHeaders(out.size());
     hpackReader.emitReferenceSet();
     checkFirstRequestWithoutHuffman();
 
-    bytesIn.set(secondRequestWithoutHuffman());
-    hpackReader.readHeaders(bytesIn.available());
+    out = secondRequestWithoutHuffman();
+    bytesIn.set(out.toByteArray());
+    hpackReader.readHeaders(out.size());
     hpackReader.emitReferenceSet();
     checkSecondRequestWithoutHuffman();
 
-    bytesIn.set(thirdRequestWithoutHuffman());
-    hpackReader.readHeaders(bytesIn.available());
+    out = thirdRequestWithoutHuffman();
+    bytesIn.set(out.toByteArray());
+    hpackReader.readHeaders(out.size());
     hpackReader.emitReferenceSet();
     checkThirdRequestWithoutHuffman();
   }
 
-  private byte[] firstRequestWithoutHuffman() {
+  private ByteArrayOutputStream firstRequestWithoutHuffman() {
     ByteArrayOutputStream out = new ByteArrayOutputStream();
 
     out.write(0x82); // == Indexed - Add ==
@@ -182,7 +180,7 @@ public class HpackDraft05Test {
     out.write(0x0f); // Literal value (len = 15)
     out.write("www.example.com".getBytes(), 0, 15);
 
-    return out.toByteArray();
+    return out;
   }
 
   private void checkFirstRequestWithoutHuffman() {
@@ -190,31 +188,19 @@ public class HpackDraft05Test {
 
     // [  1] (s =  57) :authority: www.example.com
     HpackDraft05.HeaderEntry entry = hpackReader.headerTable.get(0);
-    assertEquals(":authority", entry.name);
-    assertEquals("www.example.com", entry.value);
-    assertEquals(57, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":authority", "www.example.com", 57, true);
 
     // [  2] (s =  38) :path: /
     entry = hpackReader.headerTable.get(1);
-    assertEquals(":path", entry.name);
-    assertEquals("/", entry.value);
-    assertEquals(38, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":path", "/", 38, true);
 
     // [  3] (s =  43) :scheme: http
     entry = hpackReader.headerTable.get(2);
-    assertEquals(":scheme", entry.name);
-    assertEquals("http", entry.value);
-    assertEquals(43, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":scheme", "http", 43, true);
 
     // [  4] (s =  42) :method: GET
     entry = hpackReader.headerTable.get(3);
-    assertEquals(":method", entry.name);
-    assertEquals("GET", entry.value);
-    assertEquals(42, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":method", "GET", 42, true);
 
     // Table size: 180
     assertEquals(180, hpackReader.headerTableSize);
@@ -227,7 +213,7 @@ public class HpackDraft05Test {
         ":authority", "www.example.com"), hpackReader.getAndReset());
   }
 
-  private byte[] secondRequestWithoutHuffman() {
+  private ByteArrayOutputStream secondRequestWithoutHuffman() {
     ByteArrayOutputStream out = new ByteArrayOutputStream();
 
     out.write(0x1b); // == Literal indexed ==
@@ -235,7 +221,7 @@ public class HpackDraft05Test {
     out.write(0x08); // Literal value (len = 8)
     out.write("no-cache".getBytes(), 0, 8);
 
-    return out.toByteArray();
+    return out;
   }
 
   private void checkSecondRequestWithoutHuffman() {
@@ -243,38 +229,23 @@ public class HpackDraft05Test {
 
     // [  1] (s =  53) cache-control: no-cache
     HpackDraft05.HeaderEntry entry = hpackReader.headerTable.get(0);
-    assertEquals("cache-control", entry.name);
-    assertEquals("no-cache", entry.value);
-    assertEquals(53, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, "cache-control", "no-cache", 53, true);
 
     // [  2] (s =  57) :authority: www.example.com
     entry = hpackReader.headerTable.get(1);
-    assertEquals(":authority", entry.name);
-    assertEquals("www.example.com", entry.value);
-    assertEquals(57, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":authority", "www.example.com", 57, true);
 
     // [  3] (s =  38) :path: /
     entry = hpackReader.headerTable.get(2);
-    assertEquals(":path", entry.name);
-    assertEquals("/", entry.value);
-    assertEquals(38, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":path", "/", 38, true);
 
     // [  4] (s =  43) :scheme: http
     entry = hpackReader.headerTable.get(3);
-    assertEquals(":scheme", entry.name);
-    assertEquals("http", entry.value);
-    assertEquals(43, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":scheme", "http", 43, true);
 
     // [  5] (s =  42) :method: GET
     entry = hpackReader.headerTable.get(4);
-    assertEquals(":method", entry.name);
-    assertEquals("GET", entry.value);
-    assertEquals(42, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":method", "GET", 42, true);
 
     // Table size: 233
     assertEquals(233, hpackReader.headerTableSize);
@@ -288,7 +259,7 @@ public class HpackDraft05Test {
         "cache-control", "no-cache"), hpackReader.getAndReset());
   }
 
-  private byte[] thirdRequestWithoutHuffman() {
+  private ByteArrayOutputStream thirdRequestWithoutHuffman() {
     ByteArrayOutputStream out = new ByteArrayOutputStream();
 
     out.write(0x80); // == Empty reference set ==
@@ -306,7 +277,7 @@ public class HpackDraft05Test {
     out.write(0x0c); // Literal value (len = 12)
     out.write("custom-value".getBytes(), 0, 12);
 
-    return out.toByteArray();
+    return out;
   }
 
   private void checkThirdRequestWithoutHuffman() {
@@ -314,59 +285,35 @@ public class HpackDraft05Test {
 
     // [  1] (s =  54) custom-key: custom-value
     HpackDraft05.HeaderEntry entry = hpackReader.headerTable.get(0);
-    assertEquals("custom-key", entry.name);
-    assertEquals("custom-value", entry.value);
-    assertEquals(54, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, "custom-key", "custom-value", 54, true);
 
     // [  2] (s =  48) :path: /index.html
     entry = hpackReader.headerTable.get(1);
-    assertEquals(":path", entry.name);
-    assertEquals("/index.html", entry.value);
-    assertEquals(48, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":path", "/index.html", 48, true);
 
     // [  3] (s =  44) :scheme: https
     entry = hpackReader.headerTable.get(2);
-    assertEquals(":scheme", entry.name);
-    assertEquals("https", entry.value);
-    assertEquals(44, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":scheme", "https", 44, true);
 
     // [  4] (s =  53) cache-control: no-cache
     entry = hpackReader.headerTable.get(3);
-    assertEquals("cache-control", entry.name);
-    assertEquals("no-cache", entry.value);
-    assertEquals(53, entry.size);
-    assertFalse(entry.referenced);
+    checkEntry(entry, "cache-control", "no-cache", 53, false);
 
     // [  5] (s =  57) :authority: www.example.com
     entry = hpackReader.headerTable.get(4);
-    assertEquals(":authority", entry.name);
-    assertEquals("www.example.com", entry.value);
-    assertEquals(57, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":authority", "www.example.com", 57, true);
 
     // [  6] (s =  38) :path: /
     entry = hpackReader.headerTable.get(5);
-    assertEquals(":path", entry.name);
-    assertEquals("/", entry.value);
-    assertEquals(38, entry.size);
-    assertFalse(entry.referenced);
+    checkEntry(entry, ":path", "/", 38, false);
 
     // [  7] (s =  43) :scheme: http
     entry = hpackReader.headerTable.get(6);
-    assertEquals(":scheme", entry.name);
-    assertEquals("http", entry.value);
-    assertEquals(43, entry.size);
-    assertFalse(entry.referenced);
+    checkEntry(entry, ":scheme", "http", 43, false);
 
     // [  8] (s =  42) :method: GET
     entry = hpackReader.headerTable.get(7);
-    assertEquals(":method", entry.name);
-    assertEquals("GET", entry.value);
-    assertEquals(42, entry.size);
-    assertTrue(entry.referenced);
+    checkEntry(entry, ":method", "GET", 42, true);
 
     // Table size: 379
     assertEquals(379, hpackReader.headerTableSize);
@@ -460,10 +407,18 @@ public class HpackDraft05Test {
     return new DataInputStream(new ByteArrayInputStream(data));
   }
 
-  private byte[] literalHeaders(List<String> sentHeaders) throws IOException {
+  private ByteArrayOutputStream literalHeaders(List<String> sentHeaders) throws IOException {
     ByteArrayOutputStream headerBytes = new ByteArrayOutputStream();
     new HpackDraft05.Writer(new DataOutputStream(headerBytes)).writeHeaders(sentHeaders);
-    return headerBytes.toByteArray();
+    return headerBytes;
+  }
+
+  private void checkEntry(HpackDraft05.HeaderEntry entry, String name, String value, int size,
+      boolean referenced) {
+    assertEquals(name, entry.name);
+    assertEquals(value, entry.value);
+    assertEquals(size, entry.size);
+    assertEquals(referenced, entry.referenced);
   }
 
   private void assertBytes(int... bytes) {

okhttp/src/main/java/com/squareup/okhttp/Connection.java

@@ -70,7 +70,7 @@ public final class Connection implements Closeable {
       8, 'h', 't', 't', 'p', '/', '1', '.', '1'
   };
 
-  private static final byte[] SPDY_AND_HTTP = new byte[] {
+  private static final byte[] SPDY3_AND_HTTP11 = new byte[] {
       6, 's', 'p', 'd', 'y', '/', '3',
       8, 'h', 't', 't', 'p', '/', '1', '.', '1'
   };
@@ -157,7 +157,7 @@ public final class Connection implements Closeable {
       } else if (route.address.transports.contains("HTTP-draft-09/2.0")) {
         platform.setNpnProtocols(sslSocket, HTTP2_AND_HTTP);
       } else {
-        platform.setNpnProtocols(sslSocket, SPDY_AND_HTTP);
+        platform.setNpnProtocols(sslSocket, SPDY3_AND_HTTP11);
       }
     }
 

okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverHttp20Draft09Test.java

@@ -19,6 +19,8 @@ public class HttpOverHttp20Draft09Test extends HttpOverSpdyTest {
 
   public HttpOverHttp20Draft09Test() {
     super("HTTP-draft-09/2.0");
+    // TODO: is this really the whole authority, or just the host/port?
+    // https://github.com/http2/http2-spec/issues/334
     this.hostHeader = ":authority";
   }
 }