From ef30f7efc61ac249c1a8b43e41dba03a2bf0bef8 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Sat, 18 Jan 2014 15:54:51 -0800 Subject: [PATCH] Extract default settings to variants. Read windowSize (spdy/3) and headerTableSize (http/2) from peer. --- .../okhttp/internal/spdy/FrameWriter.java | 4 +- .../okhttp/internal/spdy/HpackDraft05.java | 53 +++++++++----- .../okhttp/internal/spdy/Http20Draft09.java | 43 +++++++++--- .../okhttp/internal/spdy/Settings.java | 35 +++++----- .../squareup/okhttp/internal/spdy/Spdy3.java | 18 ++++- .../okhttp/internal/spdy/SpdyConnection.java | 50 ++++++++----- .../okhttp/internal/spdy/SpdyStream.java | 34 +++++---- .../okhttp/internal/spdy/Variant.java | 26 +++++-- .../internal/spdy/HpackDraft05Test.java | 45 ++++++------ .../internal/spdy/Http20Draft09Test.java | 35 +++++++++- .../okhttp/internal/spdy/MockSpdyPeer.java | 8 ++- .../okhttp/internal/spdy/SettingsTest.java | 10 +-- .../internal/spdy/SpdyConnectionTest.java | 70 ++++++++++++++----- 13 files changed, 300 insertions(+), 131 deletions(-) diff --git a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/FrameWriter.java b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/FrameWriter.java index 77bbdf8c9..94abc1c43 100644 --- a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/FrameWriter.java +++ b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/FrameWriter.java @@ -25,6 +25,7 @@ import java.util.List; public interface FrameWriter extends Closeable { /** HTTP/2.0 only. */ void connectionHeader() throws IOException; + void ackSettings() throws IOException; /** SPDY/3 only. */ void flush() throws IOException; @@ -37,7 +38,8 @@ public interface FrameWriter extends Closeable { void data(boolean outFinished, int streamId, byte[] data) throws IOException; void data(boolean outFinished, int streamId, byte[] data, int offset, int byteCount) throws IOException; - void settings(Settings settings) throws IOException; + /** Write okhttp's settings to the peer. */ + void settings(Settings okHttpSettings) throws IOException; void noop() throws IOException; void ping(boolean reply, int payload1, int payload2) throws IOException; void goAway(int lastGoodStreamId, ErrorCode errorCode) throws IOException; 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 37f901f75..07d2c1ee2 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 @@ -119,6 +119,7 @@ final class HpackDraft05 { private final DataInputStream in; private final List emittedHeaders = new ArrayList(); + private int maxHeaderTableByteCount; private long bytesLeft = 0; // Visible for testing. @@ -139,13 +140,44 @@ final class HpackDraft05 { */ BitArray referencedStaticHeaders = new BitArray(); int headerTableByteCount = 0; - int maxHeaderTableByteCount = 4096; // TODO: needs to come from SETTINGS_HEADER_TABLE_SIZE. - Reader(boolean client, DataInputStream in) { + Reader(boolean client, int maxHeaderTableByteCount, DataInputStream in) { this.huffmanCodec = client ? Huffman.Codec.RESPONSE : Huffman.Codec.REQUEST; + this.maxHeaderTableByteCount = maxHeaderTableByteCount; this.in = in; } + int maxHeaderTableByteCount() { + return maxHeaderTableByteCount; + } + + /** Evicts entries as needed. */ + void maxHeaderTableByteCount(int newMaxHeaderTableByteCount) { + if (newMaxHeaderTableByteCount < headerTableByteCount) { + evictToRecoverBytes(headerTableByteCount - newMaxHeaderTableByteCount); + } + this.maxHeaderTableByteCount = newMaxHeaderTableByteCount; + } + + /** Returns the count of entries evicted. */ + private int evictToRecoverBytes(int bytesToRecover) { + int entriesToEvict = 0; + if (bytesToRecover > 0) { + // determine how many headers need to be evicted. + for (int j = headerTable.length - 1; j >= nextHeaderIndex && bytesToRecover > 0; j--) { + bytesToRecover -= headerTable[j].size; + headerTableByteCount -= headerTable[j].size; + headerCount--; + entriesToEvict++; + } + referencedHeaders.shiftLeft(entriesToEvict); + System.arraycopy(headerTable, nextHeaderIndex + 1, headerTable, + nextHeaderIndex + 1 + entriesToEvict, headerCount); + nextHeaderIndex += entriesToEvict; + } + return entriesToEvict; + } + /** * Read {@code byteCount} bytes of headers from the source stream into the * set of emitted headers. @@ -293,20 +325,7 @@ final class HpackDraft05 { // Evict headers to the required length. int bytesToRecover = (headerTableByteCount + delta) - maxHeaderTableByteCount; - int entriesToEvict = 0; - if (bytesToRecover > 0) { - // determine how many headers need to be evicted. - for (int j = headerTable.length - 1; j >= nextHeaderIndex && bytesToRecover > 0; j--) { - bytesToRecover -= headerTable[j].size; - headerTableByteCount -= headerTable[j].size; - headerCount--; - entriesToEvict++; - } - referencedHeaders.shiftLeft(entriesToEvict); - System.arraycopy(headerTable, nextHeaderIndex + 1, headerTable, - nextHeaderIndex + 1 + entriesToEvict, headerCount); - nextHeaderIndex += entriesToEvict; - } + int entriesEvicted = evictToRecoverBytes(bytesToRecover); if (index == -1) { if (headerCount + 1 > headerTable.length) { @@ -321,7 +340,7 @@ final class HpackDraft05 { headerTable[index] = entry; headerCount++; } else { // Replace value at same position. - index += headerTableIndex(index) + entriesToEvict; + index += headerTableIndex(index) + entriesEvicted; referencedHeaders.set(index); headerTable[index] = entry; } 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 eb03cc389..94108d233 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,6 +38,27 @@ public final class Http20Draft09 implements Variant { return Protocol.HTTP_2; } + // http://tools.ietf.org/html/draft-ietf-httpbis-http2-09#section-6.5 + @Override public Settings defaultOkHttpSettings(boolean client) { + Settings settings = new Settings(); + settings.set(Settings.HEADER_TABLE_SIZE, 0, 4096); + if (!client) { // client doesn't send push requests. + settings.set(Settings.ENABLE_PUSH, 0, 0); // TODO: support writing push. + } + settings.set(Settings.INITIAL_WINDOW_SIZE, 0, 65535); + return settings; + } + + @Override public Settings initialPeerSettings(boolean client) { + Settings settings = new Settings(); + settings.set(Settings.HEADER_TABLE_SIZE, 0, 4096); + if (client) { // server doesn't read push requests. + settings.set(Settings.ENABLE_PUSH, 0, 0); // TODO: support reading push. + } + settings.set(Settings.INITIAL_WINDOW_SIZE, 0, 65535); + return settings; + } + private static final byte[] CONNECTION_HEADER; static { try { @@ -66,11 +87,11 @@ public final class Http20Draft09 implements Variant { static final int FLAG_ACK = 0x1; static final int FLAG_END_FLOW_CONTROL = 0x1; - @Override public FrameReader newReader(InputStream in, boolean client) { - return new Reader(in, client); + @Override public FrameReader newReader(InputStream in, Settings peerSettings, boolean client) { + return new Reader(in, peerSettings.getHeaderTableSize(), client); } - @Override public FrameWriter newWriter(OutputStream out, boolean client) { + @Override public FrameWriter newWriter(OutputStream out, Settings ignored, boolean client) { return new Writer(out, client); } @@ -81,10 +102,10 @@ public final class Http20Draft09 implements Variant { // Visible for testing. final HpackDraft05.Reader hpackReader; - Reader(InputStream in, boolean client) { + Reader(InputStream in, int headerTableSize, boolean client) { this.in = new DataInputStream(in); this.client = client; - this.hpackReader = new HpackDraft05.Reader(client, this.in); + this.hpackReader = new HpackDraft05.Reader(client, headerTableSize, this.in); } @Override public void readConnectionHeader() throws IOException { @@ -225,9 +246,6 @@ public final class Http20Draft09 implements Variant { throws IOException { if ((flags & FLAG_ACK) != 0) { if (length != 0) throw ioException("FRAME_SIZE_ERROR ack frame should be empty!"); - // TODO: signal apply changes - // http://tools.ietf.org/html/draft-ietf-httpbis-http2-09#section-6.5.3 - return; } if (length % 8 != 0) throw ioException("TYPE_SETTINGS length %% 8 != 0: %s", length); @@ -241,6 +259,9 @@ public final class Http20Draft09 implements Variant { settings.set(id, 0, value); } handler.settings(false, settings); + if (settings.getHeaderTableSize() >= 0) { + hpackReader.maxHeaderTableByteCount(settings.getHeaderTableSize()); + } } private void readPushPromise(Handler handler, int flags, int length, int streamId) { @@ -303,6 +324,12 @@ public final class Http20Draft09 implements Variant { out.flush(); } + @Override public void ackSettings() throws IOException { + // ACK the settings frame. + out.writeInt(0 | (TYPE_SETTINGS & 0xff) << 8 | (FLAG_ACK & 0xff)); + out.writeInt(0); + } + @Override public synchronized void connectionHeader() throws IOException { if (!client) return; // Nothing to write; servers don't send connection headers! out.write(CONNECTION_HEADER); diff --git a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Settings.java b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Settings.java index f8b13ae33..f886b1b2a 100644 --- a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Settings.java +++ b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/Settings.java @@ -15,17 +15,13 @@ */ package com.squareup.okhttp.internal.spdy; -final class Settings { - /** - * spdy/3: The default initial window size for all streams is 64 KiB. (Chrome - * 25 uses 10 MiB). - */ - static final int DEFAULT_INITIAL_WINDOW_SIZE = 64 * 1024; - /** http/2: The default header compression table size is 4 KiB. */ - static final int DEFAULT_HEADER_TABLE_SIZE = 4096; - /** http/2: The default is to enable PUSH_PROMISE frames. */ - static final int DEFAULT_ENABLE_PUSH = 1; +import java.util.Arrays; +/** + * Settings describe characteristics of the sending peer, which are used by the receiving peer. + * Settings are {@link com.squareup.okhttp.internal.spdy.SpdyConnection connection} scoped. + */ +final class Settings { /** Peer request to clear durable settings. */ static final int FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS = 0x1; @@ -75,6 +71,11 @@ final class Settings { /** Flag values. */ private final int[] values = new int[COUNT]; + void clear() { + set = persistValue = persisted = 0; + Arrays.fill(values, 0); + } + void set(int id, int idFlags, int value) { if (id >= values.length) { return; // Discard unknown settings. @@ -126,11 +127,10 @@ final class Settings { return (bit & set) != 0 ? values[UPLOAD_BANDWIDTH] : defaultValue; } - /** http/2 only. */ - // TODO: honor this setting in http/2. + /** http/2 only. Returns -1 if unset. */ int getHeaderTableSize() { int bit = 1 << HEADER_TABLE_SIZE; - return (bit & set) != 0 ? values[HEADER_TABLE_SIZE] : DEFAULT_HEADER_TABLE_SIZE; + return (bit & set) != 0 ? values[HEADER_TABLE_SIZE] : -1; } /** spdy/3 only. */ @@ -141,9 +141,9 @@ final class Settings { /** http/2 only. */ // TODO: honor this setting in http/2. - boolean getEnablePush() { + boolean getEnablePush(boolean defaultValue) { int bit = 1 << ENABLE_PUSH; - return ((bit & set) != 0 ? values[ENABLE_PUSH] : DEFAULT_ENABLE_PUSH) == 1; + return ((bit & set) != 0 ? values[ENABLE_PUSH] : defaultValue ? 1 : 0) == 1; } /** spdy/3 only. */ @@ -171,9 +171,10 @@ final class Settings { } // TODO: honor this setting in http/2. - int getInitialWindowSize(int defaultValue) { + /** Returns -1 if unset. */ + int getInitialWindowSize() { int bit = 1 << INITIAL_WINDOW_SIZE; - return (bit & set) != 0 ? values[INITIAL_WINDOW_SIZE] : defaultValue; + return (bit & set) != 0 ? values[INITIAL_WINDOW_SIZE] : -1; } /** spdy/3 only. */ 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 fbfd4d469..3fc0b59fb 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,6 +36,16 @@ 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) { + Settings settings = new Settings(); + settings.set(Settings.INITIAL_WINDOW_SIZE, 0, 65535); + return settings; + } + static final int TYPE_DATA = 0x0; static final int TYPE_SYN_STREAM = 0x1; static final int TYPE_SYN_REPLY = 0x2; @@ -94,11 +104,11 @@ final class Spdy3 implements Variant { } } - @Override public FrameReader newReader(InputStream in, boolean client) { + @Override public FrameReader newReader(InputStream in, Settings ignored, boolean client) { return new Reader(in, client); } - @Override public FrameWriter newWriter(OutputStream out, boolean client) { + @Override public FrameWriter newWriter(OutputStream out, Settings ignored, boolean client) { return new Writer(out, client); } @@ -308,6 +318,10 @@ final class Spdy3 implements Variant { Platform.get().newDeflaterOutputStream(nameValueBlockBuffer, deflater, true)); } + @Override public void ackSettings() { + // Do nothing: no ACK for SPDY/3 settings. + } + @Override public synchronized void connectionHeader() { // Do nothing: no connection header for SPDY/3. } 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 bbacaab9e..e854e121f 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 @@ -72,9 +72,6 @@ public final class SpdyConnection implements Closeable { * run on the callback executor. */ private final IncomingStreamHandler handler; - private final FrameReader frameReader; - private final FrameWriter frameWriter; - private final Map streams = new HashMap(); private final String hostName; private int lastGoodStreamId; @@ -86,17 +83,24 @@ public final class SpdyConnection implements Closeable { private Map pings; private int nextPingId; - /** Lazily-created settings for the peer. */ - Settings settings; + final Settings okHttpSettings; + final Settings peerSettings; + final FrameReader frameReader; + final FrameWriter frameWriter; - ByteArrayPool bufferPool = new ByteArrayPool(8 * Settings.DEFAULT_INITIAL_WINDOW_SIZE); + final ByteArrayPool bufferPool; private SpdyConnection(Builder builder) { variant = builder.variant; 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, client); - frameWriter = variant.newWriter(builder.out, client); + frameReader = variant.newReader(builder.in, peerSettings, client); + frameWriter = variant.newWriter(builder.out, okHttpSettings, client); nextStreamId = builder.client ? 1 : 2; nextPingId = builder.client ? 1 : 2; @@ -173,7 +177,7 @@ public final class SpdyConnection implements Closeable { streamId = nextStreamId; nextStreamId += 2; stream = new SpdyStream( - streamId, this, outFinished, inFinished, priority, requestHeaders, settings); + streamId, this, outFinished, inFinished, priority, requestHeaders, peerSettings); if (stream.isOpen()) { streams.put(streamId, stream); setIdle(false); @@ -369,7 +373,7 @@ public final class SpdyConnection implements Closeable { */ public void sendConnectionHeader() throws IOException { frameWriter.connectionHeader(); - frameWriter.settings(new Settings()); + frameWriter.settings(okHttpSettings); } /** @@ -500,7 +504,7 @@ public final class SpdyConnection implements Closeable { // Create a stream. final SpdyStream newStream = new SpdyStream(streamId, SpdyConnection.this, outFinished, - inFinished, priority, nameValueBlock, settings); + inFinished, priority, nameValueBlock, peerSettings); lastGoodStreamId = streamId; streams.put(streamId, newStream); executor.submit(new NamedRunnable("OkHttp %s stream %d", hostName, streamId) { @@ -538,10 +542,13 @@ public final class SpdyConnection implements Closeable { @Override public void settings(boolean clearPrevious, Settings newSettings) { SpdyStream[] streamsToNotify = null; synchronized (SpdyConnection.this) { - if (settings == null || clearPrevious) { - settings = newSettings; + if (clearPrevious) { + peerSettings.clear(); } else { - settings.merge(newSettings); + peerSettings.merge(newSettings); + } + if (SpdyConnection.this.variant.getProtocol() == Protocol.HTTP_2) { + ackSettingsLater(); } if (!streams.isEmpty()) { streamsToNotify = streams.values().toArray(new SpdyStream[streams.size()]); @@ -550,18 +557,29 @@ public final class SpdyConnection implements Closeable { if (streamsToNotify != null) { for (SpdyStream stream : streamsToNotify) { // The synchronization here is ugly. We need to synchronize on 'this' to guard - // reads to 'settings'. We synchronize on 'stream' to guard the state change. + // reads to 'peerSettings'. We synchronize on 'stream' to guard the state change. // And we need to acquire the 'stream' lock first, since that may block. // TODO: this can block the reader thread until a write completes. That's bad! synchronized (stream) { synchronized (SpdyConnection.this) { - stream.receiveSettings(settings); + stream.receiveSettings(peerSettings); } } } } } + private void ackSettingsLater() { + executor.submit(new NamedRunnable("OkHttp %s ACK Settings", hostName) { + @Override public void execute() { + try { + frameWriter.ackSettings(); + } catch (IOException ignored) { + } + } + }); + } + @Override public void noop() { } diff --git a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyStream.java b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyStream.java index baf7e6dc1..5bb79c967 100644 --- a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyStream.java +++ b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyStream.java @@ -40,13 +40,13 @@ public final class SpdyStream { * window size, otherwise the remote peer will stop sending data on this * stream. (Chrome 25 uses 5 MiB.) */ - public static final int WINDOW_UPDATE_THRESHOLD = Settings.DEFAULT_INITIAL_WINDOW_SIZE / 2; + int windowUpdateThreshold; + private int writeWindowSize; private final int id; private final SpdyConnection connection; private final int priority; private long readTimeoutMillis = 0; - private int writeWindowSize; /** Headers sent by the stream initiator. Immutable and non null. */ private final List requestHeaders; @@ -65,19 +65,18 @@ public final class SpdyStream { private ErrorCode errorCode = null; SpdyStream(int id, SpdyConnection connection, boolean outFinished, boolean inFinished, - int priority, List requestHeaders, Settings settings) { + int priority, List requestHeaders, Settings peerSettings) { if (connection == null) throw new NullPointerException("connection == null"); if (requestHeaders == null) throw new NullPointerException("requestHeaders == null"); this.id = id; this.connection = connection; - this.in = new SpdyDataInputStream(); + this.in = new SpdyDataInputStream(peerSettings.getInitialWindowSize()); this.out = new SpdyDataOutputStream(); this.in.finished = inFinished; this.out.finished = outFinished; this.priority = priority; this.requestHeaders = requestHeaders; - - setSettings(settings); + setPeerSettings(peerSettings); } /** @@ -311,18 +310,18 @@ public final class SpdyStream { } } - private void setSettings(Settings settings) { + private void setPeerSettings(Settings peerSettings) { // TODO: For HTTP/2.0, also adjust the stream flow control window size // by the difference between the new value and the old value. assert (Thread.holdsLock(connection)); // Because 'settings' is guarded by 'connection'. - this.writeWindowSize = settings != null - ? settings.getInitialWindowSize(Settings.DEFAULT_INITIAL_WINDOW_SIZE) - : Settings.DEFAULT_INITIAL_WINDOW_SIZE; + this.writeWindowSize = peerSettings.getInitialWindowSize(); + this.windowUpdateThreshold = peerSettings.getInitialWindowSize() / 2; } - void receiveSettings(Settings settings) { + /** Notification received when peer settings change. */ + void receiveSettings(Settings peerSettings) { assert (Thread.holdsLock(this)); - setSettings(settings); + setPeerSettings(peerSettings); notifyAll(); } @@ -341,6 +340,7 @@ public final class SpdyStream { * it is not intended for use by multiple readers. */ private final class SpdyDataInputStream extends InputStream { + // Store incoming data bytes in a circular buffer. When the buffer is // empty, pos == -1. Otherwise pos is the first byte to read and limit // is the first byte to write. @@ -352,9 +352,13 @@ public final class SpdyStream { // { X X X - - - - X X X } // ^ ^ // limit pos + private final byte[] buffer; - private final byte[] buffer = SpdyStream.this.connection.bufferPool.getBuf( - Settings.DEFAULT_INITIAL_WINDOW_SIZE); + private SpdyDataInputStream(int bufferLength) { + // TODO: We probably need to change to growable buffers here pretty soon. + // Otherwise we have a performance problem where we pay for 64 KiB even if we aren't using it. + buffer = connection.bufferPool.getBuf(bufferLength); + } /** the next byte to be read, or -1 if the buffer is empty. Never buffer.length */ private int pos = -1; @@ -428,7 +432,7 @@ public final class SpdyStream { // Flow control: notify the peer that we're ready for more data! unacknowledgedBytes += copied; - if (unacknowledgedBytes >= WINDOW_UPDATE_THRESHOLD) { + if (unacknowledgedBytes >= windowUpdateThreshold) { connection.writeWindowUpdateLater(id, unacknowledgedBytes); unacknowledgedBytes = 0; } 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 b31d68613..116f9ea1e 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 @@ -28,14 +28,28 @@ interface Variant { Protocol getProtocol(); /** - * @param client true if this is the HTTP client's reader, reading frames from - * a peer SPDY or HTTP/2 server. + * Default settings used for sending frames to the peer. + * @param client true if these settings apply to writing requests, false if responses. */ - FrameReader newReader(InputStream in, boolean client); + Settings defaultOkHttpSettings(boolean client); /** - * @param client true if this is the HTTP client's writer, writing frames to a - * peer SPDY or HTTP/2 server. + * 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. */ - FrameWriter newWriter(OutputStream out, boolean client); + 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); + + /** + * @param okHttpSettings settings configured locally. + * @param client true if this is the HTTP client's writer, writing frames to a server. + */ + FrameWriter newWriter(OutputStream out, Settings okHttpSettings, boolean client); + } 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 19b1299d1..fdcbc75f9 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 @@ -37,7 +37,7 @@ public class HpackDraft05Test { private HpackDraft05.Reader hpackReader; @Before public void resetReader() { - hpackReader = new HpackDraft05.Reader(false, new DataInputStream(bytesIn)); + hpackReader = newReader(new DataInputStream(bytesIn)); } /** @@ -55,7 +55,7 @@ public class HpackDraft05Test { out.write("custom-header".getBytes(), 0, 13); bytesIn.set(out.toByteArray()); - hpackReader.maxHeaderTableByteCount = 1; + hpackReader.maxHeaderTableByteCount(1); hpackReader.readHeaders(out.size()); hpackReader.emitReferenceSet(); @@ -90,7 +90,8 @@ public class HpackDraft05Test { out.write("custom-header".getBytes(), 0, 13); bytesIn.set(out.toByteArray()); - hpackReader.maxHeaderTableByteCount = 110; + // Set to only support 110 bytes (enough for 2 headers). + hpackReader.maxHeaderTableByteCount(110); hpackReader.readHeaders(out.size()); hpackReader.emitReferenceSet(); @@ -108,6 +109,10 @@ public class HpackDraft05Test { // TODO: emit before eviction? assertEquals(byteStringList("custom-bar", "custom-header", "custom-baz", "custom-header"), hpackReader.getAndReset()); + + // Simulate receiving a small settings frame, that implies eviction. + hpackReader.maxHeaderTableByteCount(55); + assertEquals(1, hpackReader.headerCount); } /** Header table backing array is initially 8 long, let's ensure it grows. */ @@ -124,7 +129,7 @@ public class HpackDraft05Test { } bytesIn.set(out.toByteArray()); - hpackReader.maxHeaderTableByteCount = 16384; // Lots of headers need more room! + hpackReader.maxHeaderTableByteCount(16384); // Lots of headers need more room! hpackReader.readHeaders(out.size()); hpackReader.emitReferenceSet(); @@ -238,7 +243,7 @@ public class HpackDraft05Test { // idx = 2 -> :method: GET bytesIn.set(out.toByteArray()); - hpackReader.maxHeaderTableByteCount = 0; // SETTINGS_HEADER_TABLE_SIZE == 0 + hpackReader.maxHeaderTableByteCount(0); // SETTINGS_HEADER_TABLE_SIZE == 0 hpackReader.readHeaders(out.size()); hpackReader.emitReferenceSet(); @@ -675,12 +680,12 @@ public class HpackDraft05Test { new HpackDraft05.Writer(new DataOutputStream(bytesOut)); @Test public void readSingleByteInt() throws IOException { - assertEquals(10, new HpackDraft05.Reader(false, byteStream()).readInt(10, 31)); - assertEquals(10, new HpackDraft05.Reader(false, byteStream()).readInt(0xe0 | 10, 31)); + assertEquals(10, newReader(byteStream()).readInt(10, 31)); + assertEquals(10, newReader(byteStream()).readInt(0xe0 | 10, 31)); } @Test public void readMultibyteInt() throws IOException { - assertEquals(1337, new HpackDraft05.Reader(false, byteStream(154, 10)).readInt(31, 31)); + assertEquals(1337, newReader(byteStream(154, 10)).readInt(31, 31)); } @Test public void writeSingleByteInt() throws IOException { @@ -701,61 +706,59 @@ public class HpackDraft05Test { hpackWriter.writeInt(0x7fffffff, 31, 0); assertBytes(31, 224, 255, 255, 255, 7); assertEquals(0x7fffffff, - new HpackDraft05.Reader(false, byteStream(224, 255, 255, 255, 7)).readInt(31, 31)); + newReader(byteStream(224, 255, 255, 255, 7)).readInt(31, 31)); } @Test public void prefixMask() throws IOException { hpackWriter.writeInt(31, 31, 0); assertBytes(31, 0); - assertEquals(31, new HpackDraft05.Reader(false, byteStream(0)).readInt(31, 31)); + assertEquals(31, newReader(byteStream(0)).readInt(31, 31)); } @Test public void prefixMaskMinusOne() throws IOException { hpackWriter.writeInt(30, 31, 0); assertBytes(30); - assertEquals(31, new HpackDraft05.Reader(false, byteStream(0)).readInt(31, 31)); + assertEquals(31, newReader(byteStream(0)).readInt(31, 31)); } @Test public void zero() throws IOException { hpackWriter.writeInt(0, 31, 0); assertBytes(0); - assertEquals(0, new HpackDraft05.Reader(false, byteStream()).readInt(0, 31)); + assertEquals(0, newReader(byteStream()).readInt(0, 31)); } @Test public void headerName() throws IOException { hpackWriter.writeByteString(ByteString.encodeUtf8("foo")); assertBytes(3, 'f', 'o', 'o'); - assertEquals("foo", new HpackDraft05.Reader(false, byteStream(3, 'f', 'o', 'o')).readString().utf8()); + assertEquals("foo", newReader(byteStream(3, 'f', 'o', 'o')).readString().utf8()); } @Test public void emptyHeaderName() throws IOException { hpackWriter.writeByteString(ByteString.encodeUtf8("")); assertBytes(0); - assertEquals("", new HpackDraft05.Reader(false, byteStream(0)).readString().utf8()); + assertEquals("", newReader(byteStream(0)).readString().utf8()); } @Test public void headersRoundTrip() throws IOException { List sentHeaders = byteStringList("name", "value"); hpackWriter.writeHeaders(sentHeaders); ByteArrayInputStream bytesIn = new ByteArrayInputStream(bytesOut.toByteArray()); - HpackDraft05.Reader reader = new HpackDraft05.Reader(false, new DataInputStream(bytesIn)); + HpackDraft05.Reader reader = newReader(new DataInputStream(bytesIn)); reader.readHeaders(bytesOut.size()); reader.emitReferenceSet(); List receivedHeaders = reader.getAndReset(); assertEquals(sentHeaders, receivedHeaders); } + private HpackDraft05.Reader newReader(DataInputStream input) { + return new HpackDraft05.Reader(false, 4096, input); + } + private DataInputStream byteStream(int... bytes) { byte[] data = intArrayToByteArray(bytes); return new DataInputStream(new ByteArrayInputStream(data)); } - private ByteArrayOutputStream literalHeaders(List sentHeaders) throws IOException { - ByteArrayOutputStream headerBytes = new ByteArrayOutputStream(); - new HpackDraft05.Writer(new DataOutputStream(headerBytes)).writeHeaders(sentHeaders); - return headerBytes; - } - private void checkEntry(HpackDraft05.HeaderEntry entry, String name, String value, int size) { assertEquals(name, entry.name.utf8()); assertEquals(value, entry.value.utf8()); 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 4c750712c..0e9f7be05 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 @@ -47,7 +47,7 @@ public class Http20Draft09Test { dataOut.write(headerBytes); } - FrameReader fr = new Http20Draft09.Reader(new ByteArrayInputStream(out.toByteArray()), false); + FrameReader fr = newReader(out); // Consume the headers frame. fr.nextFrame(new BaseTestHandler() { @@ -92,7 +92,7 @@ public class Http20Draft09Test { dataOut.write(headerBytes); } - FrameReader fr = new Http20Draft09.Reader(new ByteArrayInputStream(out.toByteArray()), false); + FrameReader fr = newReader(out); // Reading the above frames should result in a concatenated nameValueBlock. fr.nextFrame(new BaseTestHandler() { @@ -122,7 +122,7 @@ public class Http20Draft09Test { dataOut.writeInt(expectedStreamId & 0x7fffffff); // stream with reserved bit set dataOut.writeInt(ErrorCode.COMPRESSION_ERROR.httpCode); - FrameReader fr = new Http20Draft09.Reader(new ByteArrayInputStream(out.toByteArray()), false); + FrameReader fr = newReader(out); // Consume the reset frame. fr.nextFrame(new BaseTestHandler() { @@ -133,6 +133,35 @@ public class Http20Draft09Test { }); } + @Test public void readSettingsFrame() throws IOException { + ByteArrayOutputStream out = new ByteArrayOutputStream(); + DataOutputStream dataOut = new DataOutputStream(out); + + final int reducedTableSizeBytes = 16; + + dataOut.writeShort(8); // 1 setting = 4 bytes for the code and 4 for the value. + dataOut.write(Http20Draft09.TYPE_SETTINGS); + dataOut.write(0); // No flags + dataOut.writeInt(0 & 0x7fffffff); // Settings are always on the connection stream 0. + dataOut.writeInt(Settings.HEADER_TABLE_SIZE & 0xffffff); + dataOut.writeInt(reducedTableSizeBytes); + + final Http20Draft09.Reader fr = newReader(out); + + // Consume the settings frame. + fr.nextFrame(new BaseTestHandler() { + @Override public void settings(boolean clearPrevious, Settings settings) { + assertFalse(clearPrevious); // No clearPrevious in http/2. + assertEquals(reducedTableSizeBytes, settings.getHeaderTableSize()); + } + }); + } + + private Http20Draft09.Reader newReader(ByteArrayOutputStream out) { + return new Http20Draft09.Reader(new ByteArrayInputStream(out.toByteArray()), + Variant.HTTP_20_DRAFT_09.initialPeerSettings(false).getHeaderTableSize(), false); + } + private byte[] literalHeaders(List sentHeaders) throws IOException { ByteArrayOutputStream headerBytes = new ByteArrayOutputStream(); new HpackDraft05.Writer(new DataOutputStream(headerBytes)).writeHeaders(sentHeaders); 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 c90a593f0..af3ef48ad 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 @@ -37,6 +37,7 @@ import java.util.concurrent.LinkedBlockingQueue; public final class MockSpdyPeer implements Closeable { private int frameCount = 0; private final boolean client; + private final Variant variant; private final ByteArrayOutputStream bytesOut = new ByteArrayOutputStream(); private final FrameWriter frameWriter; private final List outFrames = new ArrayList(); @@ -47,9 +48,10 @@ public final class MockSpdyPeer implements Closeable { private ServerSocket serverSocket; private Socket socket; - public MockSpdyPeer(boolean client) { + public MockSpdyPeer(Variant variant, boolean client) { this.client = client; - this.frameWriter = Variant.SPDY3.newWriter(bytesOut, client); + this.variant = variant; + this.frameWriter = variant.newWriter(bytesOut, variant.defaultOkHttpSettings(client), client); } public void acceptFrame() { @@ -109,7 +111,7 @@ public final class MockSpdyPeer implements Closeable { socket = serverSocket.accept(); OutputStream out = socket.getOutputStream(); InputStream in = socket.getInputStream(); - FrameReader reader = Variant.SPDY3.newReader(in, client); + FrameReader reader = variant.newReader(in, variant.initialPeerSettings(client), client); Iterator outFramesIterator = outFrames.iterator(); byte[] outBytes = bytesOut.toByteArray(); diff --git a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/SettingsTest.java b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/SettingsTest.java index ead73eba5..31df6d2fa 100644 --- a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/SettingsTest.java +++ b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/SettingsTest.java @@ -38,7 +38,7 @@ public final class SettingsTest { // WARNING: clash on flags between spdy/3 and http/2! assertEquals(-3, settings.getUploadBandwidth(-3)); - assertEquals(4096, settings.getHeaderTableSize()); + assertEquals(-1, settings.getHeaderTableSize()); settings.set(Settings.UPLOAD_BANDWIDTH, 0, 42); assertEquals(42, settings.getUploadBandwidth(-3)); settings.set(Settings.HEADER_TABLE_SIZE, 0, 8096); @@ -46,11 +46,11 @@ public final class SettingsTest { // WARNING: clash on flags between spdy/3 and http/2! assertEquals(-3, settings.getDownloadBandwidth(-3)); - assertTrue(settings.getEnablePush()); + assertEquals(true, settings.getEnablePush(true)); settings.set(Settings.DOWNLOAD_BANDWIDTH, 0, 53); assertEquals(53, settings.getDownloadBandwidth(-3)); settings.set(Settings.ENABLE_PUSH, 0, 0); - assertFalse(settings.getEnablePush()); + assertEquals(false, settings.getEnablePush(true)); assertEquals(-3, settings.getRoundTripTime(-3)); settings.set(Settings.ROUND_TRIP_TIME, 0, 64); @@ -68,9 +68,9 @@ public final class SettingsTest { settings.set(Settings.DOWNLOAD_RETRANS_RATE, 0, 97); assertEquals(97, settings.getDownloadRetransRate(-3)); - assertEquals(-3, settings.getInitialWindowSize(-3)); + assertEquals(-1, settings.getInitialWindowSize()); settings.set(Settings.INITIAL_WINDOW_SIZE, 0, 108); - assertEquals(108, settings.getInitialWindowSize(-3)); + assertEquals(108, settings.getInitialWindowSize()); assertEquals(-3, settings.getClientCertificateVectorSize(-3)); settings.set(Settings.CLIENT_CERTIFICATE_VECTOR_SIZE, 0, 117); 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 8d263885b..287c9be73 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 @@ -16,6 +16,7 @@ package com.squareup.okhttp.internal.spdy; +import com.squareup.okhttp.Protocol; import com.squareup.okhttp.internal.Base64; import com.squareup.okhttp.internal.Util; import java.io.ByteArrayOutputStream; @@ -45,8 +46,8 @@ import static com.squareup.okhttp.internal.spdy.Spdy3.TYPE_HEADERS; import static com.squareup.okhttp.internal.spdy.Spdy3.TYPE_NOOP; import static com.squareup.okhttp.internal.spdy.Spdy3.TYPE_PING; import static com.squareup.okhttp.internal.spdy.Spdy3.TYPE_RST_STREAM; +import static com.squareup.okhttp.internal.spdy.Spdy3.TYPE_SETTINGS; import static com.squareup.okhttp.internal.spdy.Spdy3.TYPE_WINDOW_UPDATE; -import static com.squareup.okhttp.internal.spdy.SpdyStream.WINDOW_UPDATE_THRESHOLD; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -58,7 +59,7 @@ public final class SpdyConnectionTest { throw new AssertionError(); } }; - private final MockSpdyPeer peer = new MockSpdyPeer(false); + private final MockSpdyPeer peer = new MockSpdyPeer(Variant.SPDY3, false); @After public void tearDown() throws Exception { peer.close(); @@ -253,6 +254,37 @@ public final class SpdyConnectionTest { assertEquals(4, ping4.streamId); } + @Test public void http2SettingsAck() throws Exception { + MockSpdyPeer peer = new MockSpdyPeer(Variant.HTTP_20_DRAFT_09, false); + // write the mocking script + Settings settings = new Settings(); + settings.set(Settings.HEADER_TABLE_SIZE, PERSIST_VALUE, 1024); + peer.sendFrame().settings(settings); + peer.acceptFrame(); // ACK + peer.play(); + + // play it back + SpdyConnection connection = new SpdyConnection.Builder(true, peer.openSocket()) + .protocol(Protocol.HTTP_2) + .handler(REJECT_INCOMING_STREAMS) + .build(); + + // verify the peer received the ACK + MockSpdyPeer.InFrame pingFrame = peer.takeFrame(); + assertEquals(TYPE_SETTINGS, pingFrame.type); + assertEquals(0, pingFrame.streamId); + // TODO: check for ACK flag. + assertEquals(0, pingFrame.settings.size()); + + // verify the peer's settings were read and applied. + synchronized (connection) { + assertEquals(1024, connection.peerSettings.getHeaderTableSize()); + Http20Draft09.Reader frameReader = (Http20Draft09.Reader) connection.frameReader; + assertEquals(1024, frameReader.hpackReader.maxHeaderTableByteCount()); + } + peer.close(); + } + @Test public void serverSendsSettingsToClient() throws Exception { // write the mocking script Settings settings = new Settings(); @@ -267,9 +299,9 @@ public final class SpdyConnectionTest { .handler(REJECT_INCOMING_STREAMS) .build(); - peer.takeFrame(); // Guarantees that the Settings frame has been processed. + peer.takeFrame(); // Guarantees that the peer Settings frame has been processed. synchronized (connection) { - assertEquals(10, connection.settings.getMaxConcurrentStreams(-1)); + assertEquals(10, connection.peerSettings.getMaxConcurrentStreams(-1)); } } @@ -296,14 +328,14 @@ public final class SpdyConnectionTest { peer.takeFrame(); // Guarantees that the Settings frame has been processed. synchronized (connection) { - assertEquals(100, connection.settings.getUploadBandwidth(-1)); - assertEquals(PERSIST_VALUE, connection.settings.flags(Settings.UPLOAD_BANDWIDTH)); - assertEquals(400, connection.settings.getDownloadBandwidth(-1)); - assertEquals(0, connection.settings.flags(Settings.DOWNLOAD_BANDWIDTH)); - assertEquals(500, connection.settings.getDownloadRetransRate(-1)); - assertEquals(PERSIST_VALUE, connection.settings.flags(Settings.DOWNLOAD_RETRANS_RATE)); - assertEquals(600, connection.settings.getMaxConcurrentStreams(-1)); - assertEquals(PERSIST_VALUE, connection.settings.flags(Settings.MAX_CONCURRENT_STREAMS)); + assertEquals(100, connection.peerSettings.getUploadBandwidth(-1)); + assertEquals(PERSIST_VALUE, connection.peerSettings.flags(Settings.UPLOAD_BANDWIDTH)); + assertEquals(400, connection.peerSettings.getDownloadBandwidth(-1)); + assertEquals(0, connection.peerSettings.flags(Settings.DOWNLOAD_BANDWIDTH)); + assertEquals(500, connection.peerSettings.getDownloadRetransRate(-1)); + assertEquals(PERSIST_VALUE, connection.peerSettings.flags(Settings.DOWNLOAD_RETRANS_RATE)); + assertEquals(600, connection.peerSettings.getMaxConcurrentStreams(-1)); + assertEquals(PERSIST_VALUE, connection.peerSettings.flags(Settings.MAX_CONCURRENT_STREAMS)); } } @@ -931,11 +963,12 @@ public final class SpdyConnectionTest { } @Test public void readSendsWindowUpdate() throws Exception { + int windowUpdateThreshold = Variant.SPDY3.initialPeerSettings(true).getInitialWindowSize() / 2; // Write the mocking script. peer.acceptFrame(); // SYN_STREAM peer.sendFrame().synReply(false, 1, byteStringList("a", "android")); for (int i = 0; i < 3; i++) { - peer.sendFrame().data(false, 1, new byte[WINDOW_UPDATE_THRESHOLD]); + peer.sendFrame().data(false, 1, new byte[windowUpdateThreshold]); peer.acceptFrame(); // WINDOW UPDATE } peer.sendFrame().data(true, 1, new byte[0]); @@ -944,6 +977,7 @@ public final class SpdyConnectionTest { // Play it back. SpdyConnection connection = new SpdyConnection.Builder(true, peer.openSocket()).build(); SpdyStream stream = connection.newStream(byteStringList("b", "banana"), true, true); + assertEquals(windowUpdateThreshold, stream.windowUpdateThreshold); assertEquals(byteStringList("a", "android"), stream.getResponseHeaders()); InputStream in = stream.getInputStream(); int total = 0; @@ -951,7 +985,7 @@ public final class SpdyConnectionTest { int count; while ((count = in.read(buffer)) != -1) { total += count; - if (total == 3 * WINDOW_UPDATE_THRESHOLD) break; + if (total == 3 * windowUpdateThreshold) break; } assertEquals(-1, in.read()); @@ -962,14 +996,16 @@ public final class SpdyConnectionTest { MockSpdyPeer.InFrame windowUpdate = peer.takeFrame(); assertEquals(TYPE_WINDOW_UPDATE, windowUpdate.type); assertEquals(1, windowUpdate.streamId); - assertEquals(WINDOW_UPDATE_THRESHOLD, windowUpdate.deltaWindowSize); + assertEquals(windowUpdateThreshold, windowUpdate.deltaWindowSize); } } @Test public void writeAwaitsWindowUpdate() throws Exception { + int windowSize = Variant.SPDY3.initialPeerSettings(true).getInitialWindowSize(); + // Write the mocking script. This accepts more data frames than necessary! peer.acceptFrame(); // SYN_STREAM - for (int i = 0; i < Settings.DEFAULT_INITIAL_WINDOW_SIZE / 1024; i++) { + for (int i = 0; i < windowSize / 1024; i++) { peer.acceptFrame(); // DATA } peer.play(); @@ -978,7 +1014,7 @@ public final class SpdyConnectionTest { SpdyConnection connection = new SpdyConnection.Builder(true, peer.openSocket()).build(); SpdyStream stream = connection.newStream(byteStringList("b", "banana"), true, true); OutputStream out = stream.getOutputStream(); - out.write(new byte[Settings.DEFAULT_INITIAL_WINDOW_SIZE]); + out.write(new byte[windowSize]); interruptAfterDelay(500); try { out.write('a');