From 04e1002ad11b41d6483e614c673cfaa81ae1a551 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Fri, 17 Jan 2014 13:18:45 -0800 Subject: [PATCH] Support more than 64 referenced headers in HPACK. --- .../squareup/okhttp/internal/BitArray.java | 10 +--- .../okhttp/internal/spdy/HpackDraft05.java | 51 +++++++------------ .../okhttp/internal/BitArrayTest.java | 27 ---------- .../internal/spdy/HpackDraft05Test.java | 38 ++++---------- 4 files changed, 28 insertions(+), 98 deletions(-) diff --git a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/BitArray.java b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/BitArray.java index b453afe6a..3e700ad4a 100644 --- a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/BitArray.java +++ b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/BitArray.java @@ -21,10 +21,6 @@ import java.util.List; /** A simple bitset which supports left shifting. */ public final class BitArray { - /** Create a bit array from a bit-packed long. */ - public static BitArray fromValue(long bitLong) { - return new BitArray(bitLong); - } long[] data; @@ -32,14 +28,10 @@ public final class BitArray { // offset the outward facing index to support shifts without having to move the underlying bits. private int start; // Valid values are [0..63] - BitArray() { + public BitArray() { data = new long[1]; } - private BitArray(long bitLong) { - data = new long[] { bitLong, 0 }; - } - private void growToSize(int size) { long[] newData = new long[size]; if (data != null) { diff --git a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/HpackDraft05.java b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/HpackDraft05.java index 482c286dd..37f901f75 100644 --- a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/HpackDraft05.java +++ b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/HpackDraft05.java @@ -1,5 +1,6 @@ package com.squareup.okhttp.internal.spdy; +import com.squareup.okhttp.internal.BitArray; import com.squareup.okhttp.internal.ByteString; import com.squareup.okhttp.internal.Util; import java.io.DataInputStream; @@ -16,9 +17,7 @@ import java.util.List; * * This implementation uses an array for the header table with a bitset for * references. Dynamic entries are added to the array, starting in the last - * position moving forward. When the array fills, it is doubled, up to the - * supported maximum of 64 headers. HTTP requests or responses that require - * more than 64 headers are hence not currently supported. + * position moving forward. When the array fills, it is doubled. */ final class HpackDraft05 { @@ -123,7 +122,7 @@ final class HpackDraft05 { private long bytesLeft = 0; // Visible for testing. - HeaderEntry[] headerTable = new HeaderEntry[8]; // must be less than 64 + HeaderEntry[] headerTable = new HeaderEntry[8]; // Array is populated back to front, so new entries always have lowest index. int nextHeaderIndex = headerTable.length - 1; int headerCount = 0; @@ -131,15 +130,14 @@ final class HpackDraft05 { /** * Set bit positions indicate {@code headerTable[pos]} should be emitted. */ - // Using a long since the reference table < 64 entries. - long referencedHeaders = 0x0000000000000000L; + // Using a BitArray as it has left-shift operator. + BitArray referencedHeaders = new BitArray(); /** * Set bit positions indicate {@code STATIC_HEADER_TABLE[pos]} should be * emitted. */ - // Using a long since the static table < 64 entries. - long referencedStaticHeaders = 0x0000000000000000L; + BitArray referencedStaticHeaders = new BitArray(); int headerTableByteCount = 0; int maxHeaderTableByteCount = 4096; // TODO: needs to come from SETTINGS_HEADER_TABLE_SIZE. @@ -184,19 +182,19 @@ final class HpackDraft05 { } private void clearReferenceSet() { - referencedStaticHeaders = 0x0000000000000000L; - referencedHeaders = 0x0000000000000000L; + referencedStaticHeaders.clear(); + referencedHeaders.clear(); } public void emitReferenceSet() { for (int i = 0; i < STATIC_HEADER_TABLE.length; ++i) { - if (bitPositionSet(referencedStaticHeaders, i)) { + if (referencedStaticHeaders.get(i)) { emittedHeaders.add(STATIC_HEADER_TABLE[i].name); emittedHeaders.add(STATIC_HEADER_TABLE[i].value); } } for (int i = headerTable.length - 1; i != nextHeaderIndex; --i) { - if (bitPositionSet(referencedHeaders, i)) { + if (referencedHeaders.get(i)) { emittedHeaders.add(headerTable[i].name); emittedHeaders.add(headerTable[i].value); } @@ -214,17 +212,15 @@ final class HpackDraft05 { } private void readIndexedHeader(int index) { - if (isStaticHeader(index)) { if (maxHeaderTableByteCount == 0) { - // Set bit designating this static entry is referenced. - referencedStaticHeaders |= (1L << (index - headerCount)); + referencedStaticHeaders.set(index - headerCount); } else { HeaderEntry staticEntry = STATIC_HEADER_TABLE[index - headerCount]; insertIntoHeaderTable(-1, staticEntry); } - } else if (!bitPositionSet(referencedHeaders, headerTableIndex(index))) { - referencedHeaders |= (1L << headerTableIndex(index)); + } else if (!referencedHeaders.get(headerTableIndex(index))) { + referencedHeaders.set(headerTableIndex(index)); } else { // TODO: we should throw something that we can coerce to a PROTOCOL_ERROR throw new AssertionError("invalid index " + index); @@ -284,8 +280,7 @@ final class HpackDraft05 { // if the new or replacement header is too big, drop all entries. if (delta > maxHeaderTableByteCount) { - referencedStaticHeaders = 0x0000000000000000L; - referencedHeaders = 0x0000000000000000L; + clearReferenceSet(); Arrays.fill(headerTable, null); nextHeaderIndex = headerTable.length - 1; headerCount = 0; @@ -307,8 +302,7 @@ final class HpackDraft05 { headerCount--; entriesToEvict++; } - // shift elements over - referencedHeaders = referencedHeaders << entriesToEvict; + referencedHeaders.shiftLeft(entriesToEvict); System.arraycopy(headerTable, nextHeaderIndex + 1, headerTable, nextHeaderIndex + 1 + entriesToEvict, headerCount); nextHeaderIndex += entriesToEvict; @@ -316,24 +310,19 @@ final class HpackDraft05 { if (index == -1) { if (headerCount + 1 > headerTable.length) { - if (headerTable.length == 64) { - // We would need to switch off long to bitset to support > 64 headers. - throw new UnsupportedOperationException( - "Header tables with count > 64 not yet supported!"); - } HeaderEntry[] doubled = new HeaderEntry[headerTable.length * 2]; System.arraycopy(headerTable, 0, doubled, headerTable.length, headerTable.length); - referencedHeaders = referencedHeaders << headerTable.length; + referencedHeaders.shiftLeft(headerTable.length); nextHeaderIndex = headerTable.length - 1; headerTable = doubled; } index = nextHeaderIndex--; - referencedHeaders |= (1L << index); + referencedHeaders.set(index); headerTable[index] = entry; headerCount++; } else { // Replace value at same position. index += headerTableIndex(index) + entriesToEvict; - referencedHeaders |= (1L << index); + referencedHeaders.set(index); headerTable[index] = entry; } headerTableByteCount += delta; @@ -385,10 +374,6 @@ final class HpackDraft05 { } } - static boolean bitPositionSet(long referenceBitSet, int i) { - return ((referenceBitSet >> i) & 1L) == 1; - } - static class Writer { private final OutputStream out; diff --git a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/BitArrayTest.java b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/BitArrayTest.java index 85ba915e9..5ecc9d7e6 100644 --- a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/BitArrayTest.java +++ b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/BitArrayTest.java @@ -20,36 +20,9 @@ import org.junit.Test; import static java.util.Arrays.asList; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; public class BitArrayTest { - @Test public void initializeFromLong() { - long bits = 1L | (1L << 3) | (1L << 7); - BitArray b = BitArray.fromValue(bits); - assertTrue(b.get(0)); - assertTrue(b.get(3)); - assertTrue(b.get(7)); - } - - @Test public void initializedFullFromLong() { - long bits = -1; - BitArray b = BitArray.fromValue(bits); - for (int i = 0; i < 64; i++) { - assertTrue(b.get(i)); - } - } - - @Test public void hpackUseCase() { - long bits = 1L | (1L << 63); - BitArray b = BitArray.fromValue(bits); - assertTrue(b.get(0)); - assertFalse(b.get(1)); - assertTrue(b.get(63)); - assertFalse(b.get(64)); - b.set(64); - assertTrue(b.get(64)); - } @Test public void setExpandsData() { BitArray b = new BitArray(); diff --git a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/HpackDraft05Test.java b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/HpackDraft05Test.java index 2ddb84315..19b1299d1 100644 --- a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/HpackDraft05Test.java +++ b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/HpackDraft05Test.java @@ -27,11 +27,9 @@ import org.junit.Before; import org.junit.Test; import static com.squareup.okhttp.internal.Util.byteStringList; -import static com.squareup.okhttp.internal.spdy.HpackDraft05.bitPositionSet; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; public class HpackDraft05Test { @@ -113,10 +111,10 @@ public class HpackDraft05Test { } /** Header table backing array is initially 8 long, let's ensure it grows. */ - @Test public void dynamicallyGrowsUpTo64Entries() throws IOException { + @Test public void dynamicallyGrowsBeyond64Entries() throws IOException { ByteArrayOutputStream out = new ByteArrayOutputStream(); - for (int i = 0; i < 64; i++) { + for (int i = 0; i < 256; i++) { out.write(0x00); // Literal indexed out.write(0x0a); // Literal name (len = 10) out.write("custom-foo".getBytes(), 0, 10); @@ -126,34 +124,16 @@ public class HpackDraft05Test { } bytesIn.set(out.toByteArray()); + hpackReader.maxHeaderTableByteCount = 16384; // Lots of headers need more room! hpackReader.readHeaders(out.size()); hpackReader.emitReferenceSet(); - assertEquals(64, hpackReader.headerCount); + assertEquals(256, hpackReader.headerCount); + assertHeaderReferenced(headerTableLength() - 1); + assertHeaderReferenced(headerTableLength() - hpackReader.headerCount); } - @Test public void greaterThan64HeadersNotYetSupported() throws IOException { - ByteArrayOutputStream out = new ByteArrayOutputStream(); - - for (int i = 0; i < 65; i++) { - out.write(0x00); // Literal indexed - out.write(0x0a); // Literal name (len = 10) - out.write("custom-foo".getBytes(), 0, 10); - - out.write(0x0d); // Literal value (len = 13) - out.write("custom-header".getBytes(), 0, 13); - } - - bytesIn.set(out.toByteArray()); - try { - hpackReader.readHeaders(out.size()); - fail(); - } catch (UnsupportedOperationException expected) { - } - } - - /** Huffman headers are accepted, but come out as garbage for now. */ - @Test public void huffmanDecodingNotYetSupported() throws IOException { + @Test public void huffmanDecodingSupported() throws IOException { ByteArrayOutputStream out = new ByteArrayOutputStream(); out.write(0x04); // == Literal indexed == @@ -798,11 +778,11 @@ public class HpackDraft05Test { } private void assertHeaderReferenced(int index) { - assertTrue(bitPositionSet(hpackReader.referencedHeaders, index)); + assertTrue(hpackReader.referencedHeaders.get(index)); } private void assertHeaderNotReferenced(int index) { - assertFalse(bitPositionSet(hpackReader.referencedHeaders, index)); + assertFalse(hpackReader.referencedHeaders.get(index)); } private int headerTableLength() {