From 6171f554512ce314d5b7008fc660a1697887581c Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Tue, 17 Jun 2014 22:25:19 -0400 Subject: [PATCH] Fix two problems with the window size. Each peer has a window size. The peer's window size controls how much data we can send the peer, and our window size controls how much data the peer can send us. We had these reversed in SpdyStream, so if the peer specified a non-default window size we wouldn't send the required WINDOW_UPDATE events when we needed to. The connection has its own window size. But unlike the stream window sizes, this needs to be manually adjusted with a windowUpdate frame when the connection is opened. Closes https://github.com/square/okhttp/issues/933 --- .../internal/spdy/Http2ConnectionTest.java | 18 +++++++++--------- .../internal/spdy/Spdy3ConnectionTest.java | 18 +++++++++--------- .../okhttp/internal/spdy/SpdyConnection.java | 7 ++++++- .../okhttp/internal/spdy/SpdyStream.java | 4 ++-- 4 files changed, 26 insertions(+), 21 deletions(-) diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/Http2ConnectionTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/Http2ConnectionTest.java index 5bcb09e38..e700dd6c6 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/Http2ConnectionTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/Http2ConnectionTest.java @@ -211,17 +211,17 @@ public final class Http2ConnectionTest { @Test public void readSendsWindowUpdateHttp2() throws Exception { peer.setVariantAndClient(HTTP_2, false); - int windowUpdateThreshold = DEFAULT_INITIAL_WINDOW_SIZE / 2; + int windowSize = 100; + int windowUpdateThreshold = 50; // Write the mocking script. peer.acceptFrame(); // SYN_STREAM peer.sendFrame().synReply(false, 3, headerEntries("a", "android")); for (int i = 0; i < 3; i++) { - // Send frames summing to windowUpdateThreshold. - for (int sent = 0, count; sent < windowUpdateThreshold; sent += count) { - count = Math.min(HTTP_2.maxFrameSize(), windowUpdateThreshold - sent); - peer.sendFrame().data(false, 3, data(count)); - } + // Send frames of summing to size 50, which is windowUpdateThreshold. + peer.sendFrame().data(false, 3, data(24)); + peer.sendFrame().data(false, 3, data(25)); + peer.sendFrame().data(false, 3, data(1)); peer.acceptFrame(); // connection WINDOW UPDATE peer.acceptFrame(); // stream WINDOW UPDATE } @@ -230,15 +230,15 @@ public final class Http2ConnectionTest { // Play it back. SpdyConnection connection = connection(peer, HTTP_2); + connection.okHttpSettings.set(Settings.INITIAL_WINDOW_SIZE, 0, windowSize); SpdyStream stream = connection.newStream(headerEntries("b", "banana"), false, true); assertEquals(0, stream.unacknowledgedBytesRead); assertEquals(headerEntries("a", "android"), stream.getResponseHeaders()); Source in = stream.getSource(); Buffer buffer = new Buffer(); - while (in.read(buffer, 1024) != -1) { - if (buffer.size() == 3 * windowUpdateThreshold) break; - } + buffer.writeAll(in); assertEquals(-1, in.read(buffer, 1)); + assertEquals(150, buffer.size()); MockSpdyPeer.InFrame synStream = peer.takeFrame(); assertEquals(TYPE_HEADERS, synStream.type); diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/Spdy3ConnectionTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/Spdy3ConnectionTest.java index 862e2f34a..a6c970362 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/Spdy3ConnectionTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/Spdy3ConnectionTest.java @@ -1013,17 +1013,17 @@ public final class Spdy3ConnectionTest { @Test public void readSendsWindowUpdate() throws Exception { peer.setVariantAndClient(SPDY3, false); - int windowUpdateThreshold = DEFAULT_INITIAL_WINDOW_SIZE / 2; + int windowSize = 100; + int windowUpdateThreshold = 50; // Write the mocking script. peer.acceptFrame(); // SYN_STREAM peer.sendFrame().synReply(false, 1, headerEntries("a", "android")); for (int i = 0; i < 3; i++) { - // Send frames summing to windowUpdateThreshold. - for (int sent = 0, count; sent < windowUpdateThreshold; sent += count) { - count = Math.min(SPDY3.maxFrameSize(), windowUpdateThreshold - sent); - peer.sendFrame().data(false, 1, data(count)); - } + // Send frames of summing to size 50, which is windowUpdateThreshold. + peer.sendFrame().data(false, 1, data(24)); + peer.sendFrame().data(false, 1, data(25)); + peer.sendFrame().data(false, 1, data(1)); peer.acceptFrame(); // connection WINDOW UPDATE peer.acceptFrame(); // stream WINDOW UPDATE } @@ -1032,15 +1032,15 @@ public final class Spdy3ConnectionTest { // Play it back. SpdyConnection connection = connection(peer, SPDY3); + connection.okHttpSettings.set(Settings.INITIAL_WINDOW_SIZE, 0, windowSize); SpdyStream stream = connection.newStream(headerEntries("b", "banana"), false, true); assertEquals(0, stream.unacknowledgedBytesRead); assertEquals(headerEntries("a", "android"), stream.getResponseHeaders()); Source in = stream.getSource(); Buffer buffer = new Buffer(); - while (in.read(buffer, 1024) != -1) { - if (buffer.size() == 3 * windowUpdateThreshold) break; - } + buffer.writeAll(in); assertEquals(-1, in.read(buffer, 1)); + assertEquals(150, buffer.size()); MockSpdyPeer.InFrame synStream = peer.takeFrame(); assertEquals(TYPE_HEADERS, synStream.type); diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java b/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java index bceb25784..8e22f702c 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java @@ -113,6 +113,7 @@ public final class SpdyConnection implements Closeable { // TODO: Do we want to dynamically adjust settings, or KISS and only set once? final Settings okHttpSettings = new Settings(); // okHttpSettings.set(Settings.MAX_CONCURRENT_STREAMS, 0, max); + private static final int OKHTTP_CLIENT_WINDOW_SIZE = 16 * 1024 * 1024; /** Settings we receive from the peer. */ // TODO: MWS will need to guard on this setting before attempting to push. @@ -145,7 +146,7 @@ public final class SpdyConnection implements Closeable { // thrashing window updates every 64KiB, yet small enough to avoid blowing // up the heap. if (builder.client) { - okHttpSettings.set(Settings.INITIAL_WINDOW_SIZE, 0, 16 * 1024 * 1024); + okHttpSettings.set(Settings.INITIAL_WINDOW_SIZE, 0, OKHTTP_CLIENT_WINDOW_SIZE); } hostName = builder.hostName; @@ -503,6 +504,10 @@ public final class SpdyConnection implements Closeable { public void sendConnectionPreface() throws IOException { frameWriter.connectionPreface(); frameWriter.settings(okHttpSettings); + int windowSize = okHttpSettings.getInitialWindowSize(Settings.DEFAULT_INITIAL_WINDOW_SIZE); + if (windowSize != Settings.DEFAULT_INITIAL_WINDOW_SIZE) { + frameWriter.windowUpdate(0, windowSize - Settings.DEFAULT_INITIAL_WINDOW_SIZE); + } } public static class Builder { diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyStream.java b/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyStream.java index 7eb78cbef..331536d37 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyStream.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyStream.java @@ -350,7 +350,7 @@ public final class SpdyStream { // Flow control: notify the peer that we're ready for more data! unacknowledgedBytesRead += read; if (unacknowledgedBytesRead - >= connection.peerSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE) / 2) { + >= connection.okHttpSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE) / 2) { connection.writeWindowUpdateLater(id, unacknowledgedBytesRead); unacknowledgedBytesRead = 0; } @@ -360,7 +360,7 @@ public final class SpdyStream { synchronized (connection) { // Multiple application threads may hit this section. connection.unacknowledgedBytesRead += read; if (connection.unacknowledgedBytesRead - >= connection.peerSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE) / 2) { + >= connection.okHttpSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE) / 2) { connection.writeWindowUpdateLater(0, connection.unacknowledgedBytesRead); connection.unacknowledgedBytesRead = 0; }