Repository: square/okhttp
Issue: SpdyConnection clears the old settings without merging with the new server settings causing the window size always set as -1

When debugging an issue with client requests hanging on SpdyStream.waitUntilWritable(), we found that the window size is set as -1 in SpdyStream.java at line: 

this.bytesLeftInWriteWindow = connection.peerSettings.getInitialWindowSize();

After tracing down the issue, the problem turned out to be line 662 of https://github.com/square/okhttp/blob/master/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java, where peerSettings is cleared without merging with the new settings. This has caused any later SpdyStream always uses an –1 as the initial window size, and the bug has also caused the new window size settings not get propagated to existing SpdySteam(s). 


Changed files:
okhttp/src/main/java/com/squareup/okhttp/internal/spdy/Settings.java
okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/SettingsTest.java
okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java
okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/SpdyConnectionTest.java
okhttp/src/main/java/com/squareup/okhttp/internal/spdy/SpdyStream.java

diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/SettingsTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/SettingsTest.java
index 44459b8c9..294684f1b 100644
--- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/SettingsTest.java
+++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/SettingsTest.java
@@ -17,6 +17,7 @@ package com.squareup.okhttp.internal.spdy;
 
 import org.junit.Test;
 
+import static com.squareup.okhttp.internal.spdy.Settings.DEFAULT_INITIAL_WINDOW_SIZE;
 import static com.squareup.okhttp.internal.spdy.Settings.DOWNLOAD_BANDWIDTH;
 import static com.squareup.okhttp.internal.spdy.Settings.DOWNLOAD_RETRANS_RATE;
 import static com.squareup.okhttp.internal.spdy.Settings.MAX_CONCURRENT_STREAMS;
@@ -68,9 +69,10 @@ public final class SettingsTest {
     settings.set(DOWNLOAD_RETRANS_RATE, 0, 97);
     assertEquals(97, settings.getDownloadRetransRate(-3));
 
-    assertEquals(-1, settings.getInitialWindowSize());
+    assertEquals(DEFAULT_INITIAL_WINDOW_SIZE,
+        settings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE));
     settings.set(Settings.INITIAL_WINDOW_SIZE, 0, 108);
-    assertEquals(108, settings.getInitialWindowSize());
+    assertEquals(108, settings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE));
 
     assertEquals(-3, settings.getClientCertificateVectorSize(-3));
     settings.set(Settings.CLIENT_CERTIFICATE_VECTOR_SIZE, 0, 117);
diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/SpdyConnectionTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/SpdyConnectionTest.java
index 8b934fa16..b73d6f930 100644
--- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/SpdyConnectionTest.java
+++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/spdy/SpdyConnectionTest.java
@@ -39,6 +39,7 @@ import static com.squareup.okhttp.internal.spdy.ErrorCode.INVALID_STREAM;
 import static com.squareup.okhttp.internal.spdy.ErrorCode.PROTOCOL_ERROR;
 import static com.squareup.okhttp.internal.spdy.ErrorCode.REFUSED_STREAM;
 import static com.squareup.okhttp.internal.spdy.ErrorCode.STREAM_IN_USE;
+import static com.squareup.okhttp.internal.spdy.Settings.DEFAULT_INITIAL_WINDOW_SIZE;
 import static com.squareup.okhttp.internal.spdy.Settings.PERSIST_VALUE;
 import static com.squareup.okhttp.internal.spdy.Spdy3.TYPE_DATA;
 import static com.squareup.okhttp.internal.spdy.Spdy3.TYPE_GOAWAY;
@@ -47,7 +48,6 @@ 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.SpdyConnection.INITIAL_WINDOW_SIZE;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -301,7 +301,7 @@ public final class SpdyConnectionTest {
     // This stream was created *after* the connection settings were adjusted.
     SpdyStream stream = connection.newStream(headerEntries("a", "android"), false, true);
 
-    assertEquals(3368, connection.peerSettings.getInitialWindowSize());
+    assertEquals(3368, connection.peerSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE));
     assertEquals(1684, connection.bytesLeftInWriteWindow); // initial wasn't affected.
     // New Stream is has the most recent initial window size.
     assertEquals(3368, stream.bytesLeftInWriteWindow);
@@ -405,6 +405,35 @@ public final class SpdyConnectionTest {
     }
   }
 
+  @Test public void clearSettingsBeforeMerge() throws Exception {
+    // write the mocking script
+    Settings settings1 = new Settings();
+    settings1.set(Settings.UPLOAD_BANDWIDTH, PERSIST_VALUE, 100);
+    settings1.set(Settings.DOWNLOAD_BANDWIDTH, PERSIST_VALUE, 200);
+    settings1.set(Settings.DOWNLOAD_RETRANS_RATE, 0, 300);
+    peer.sendFrame().settings(settings1);
+    peer.sendFrame().ping(false, 2, 0);
+    peer.acceptFrame();
+    peer.play();
+
+    // play it back
+    SpdyConnection connection = connection(peer, SPDY3);
+
+    peer.takeFrame(); // Guarantees that the Settings frame has been processed.
+
+    // fake a settings frame with clear flag set.
+    Settings settings2 = new Settings();
+    settings2.set(Settings.MAX_CONCURRENT_STREAMS, PERSIST_VALUE, 600);
+    connection.readerRunnable.settings(true, settings2);
+
+    synchronized (connection) {
+      assertEquals(-1, connection.peerSettings.getUploadBandwidth(-1));
+      assertEquals(-1, connection.peerSettings.getDownloadBandwidth(-1));
+      assertEquals(-1, connection.peerSettings.getDownloadRetransRate(-1));
+      assertEquals(600, connection.peerSettings.getMaxConcurrentStreams(-1));
+    }
+  }
+
   @Test public void bogusDataFrameDoesNotDisruptConnection() throws Exception {
     // write the mocking script
     peer.sendFrame().data(true, 41, new OkBuffer().writeUtf8("bogus"));
@@ -1042,7 +1071,7 @@ public final class SpdyConnectionTest {
       throws IOException, InterruptedException {
     peer.setVariantAndClient(variant, false);
 
-    int windowUpdateThreshold = INITIAL_WINDOW_SIZE / 2;
+    int windowUpdateThreshold = DEFAULT_INITIAL_WINDOW_SIZE / 2;
 
     // Write the mocking script.
     peer.acceptFrame(); // SYN_STREAM
@@ -1152,7 +1181,7 @@ public final class SpdyConnectionTest {
   }
 
   @Test public void writeAwaitsWindowUpdate() throws Exception {
-    int framesThatFillWindow = roundUp(INITIAL_WINDOW_SIZE, HTTP_20_DRAFT_09.maxFrameSize());
+    int framesThatFillWindow = roundUp(DEFAULT_INITIAL_WINDOW_SIZE, HTTP_20_DRAFT_09.maxFrameSize());
 
     // Write the mocking script. This accepts more data frames than necessary!
     peer.acceptFrame(); // SYN_STREAM
@@ -1166,7 +1195,7 @@ public final class SpdyConnectionTest {
     SpdyConnection connection = connection(peer, SPDY3);
     SpdyStream stream = connection.newStream(headerEntries("b", "banana"), true, true);
     BufferedSink out = Okio.buffer(stream.getSink());
-    out.write(new byte[INITIAL_WINDOW_SIZE]);
+    out.write(new byte[DEFAULT_INITIAL_WINDOW_SIZE]);
     out.flush();
 
     // Check that we've filled the window for both the stream and also the connection.
@@ -1194,7 +1223,7 @@ public final class SpdyConnectionTest {
   }
 
   @Test public void initialSettingsWithWindowSizeAdjustsConnection() throws Exception {
-    int framesThatFillWindow = roundUp(INITIAL_WINDOW_SIZE, HTTP_20_DRAFT_09.maxFrameSize());
+    int framesThatFillWindow = roundUp(DEFAULT_INITIAL_WINDOW_SIZE, HTTP_20_DRAFT_09.maxFrameSize());
 
     // Write the mocking script. This accepts more data frames than necessary!
     peer.acceptFrame(); // SYN_STREAM
@@ -1208,7 +1237,7 @@ public final class SpdyConnectionTest {
     SpdyConnection connection = connection(peer, SPDY3);
     SpdyStream stream = connection.newStream(headerEntries("a", "apple"), true, true);
     BufferedSink out = Okio.buffer(stream.getSink());
-    out.write(new byte[INITIAL_WINDOW_SIZE]);
+    out.write(new byte[DEFAULT_INITIAL_WINDOW_SIZE]);
     out.flush();
 
     // write 1 more than the window size
@@ -1221,7 +1250,7 @@ public final class SpdyConnectionTest {
 
     // Receiving a Settings with a larger window size will unblock the streams.
     Settings initial = new Settings();
-    initial.set(Settings.INITIAL_WINDOW_SIZE, PERSIST_VALUE, INITIAL_WINDOW_SIZE + 1);
+    initial.set(Settings.INITIAL_WINDOW_SIZE, PERSIST_VALUE, DEFAULT_INITIAL_WINDOW_SIZE + 1);
     connection.readerRunnable.settings(false, initial);
 
     assertEquals(1, connection.bytesLeftInWriteWindow);
@@ -1235,7 +1264,7 @@ public final class SpdyConnectionTest {
 
     // Settings after the initial do not affect the connection window size.
     Settings next = new Settings();
-    next.set(Settings.INITIAL_WINDOW_SIZE, PERSIST_VALUE, INITIAL_WINDOW_SIZE + 2);
+    next.set(Settings.INITIAL_WINDOW_SIZE, PERSIST_VALUE, DEFAULT_INITIAL_WINDOW_SIZE + 2);
     connection.readerRunnable.settings(false, next);
 
     assertEquals(0, connection.bytesLeftInWriteWindow); // connection wasn't affected.
@@ -1263,7 +1292,7 @@ public final class SpdyConnectionTest {
   }
 
   @Test public void blockedStreamDoesntStarveNewStream() throws Exception {
-    int framesThatFillWindow = roundUp(INITIAL_WINDOW_SIZE, SPDY3.maxFrameSize());
+    int framesThatFillWindow = roundUp(DEFAULT_INITIAL_WINDOW_SIZE, SPDY3.maxFrameSize());
 
     // Write the mocking script. This accepts more data frames than necessary!
     peer.acceptFrame(); // SYN_STREAM on stream 1
@@ -1278,7 +1307,7 @@ public final class SpdyConnectionTest {
     SpdyConnection connection = connection(peer, SPDY3);
     SpdyStream stream1 = connection.newStream(headerEntries("a", "apple"), true, true);
     BufferedSink out1 = Okio.buffer(stream1.getSink());
-    out1.write(new byte[INITIAL_WINDOW_SIZE]);
+    out1.write(new byte[DEFAULT_INITIAL_WINDOW_SIZE]);
     out1.flush();
 
     // Check that we've filled the window for both the stream and also the connection.
@@ -1299,7 +1328,7 @@ public final class SpdyConnectionTest {
 
     assertEquals(0, connection.bytesLeftInWriteWindow);
     assertEquals(0, connection.getStream(1).bytesLeftInWriteWindow);
-    assertEquals(INITIAL_WINDOW_SIZE - 3, connection.getStream(3).bytesLeftInWriteWindow);
+    assertEquals(DEFAULT_INITIAL_WINDOW_SIZE - 3, connection.getStream(3).bytesLeftInWriteWindow);
   }
 
   @Test public void maxFrameSizeHonored() throws Exception {
diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/Settings.java b/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/Settings.java
index c05d6b174..bf430882e 100644
--- a/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/Settings.java
+++ b/okhttp/src/main/java/com/squareup/okhttp/internal/spdy/Settings.java
@@ -22,6 +22,12 @@ import java.util.Arrays;
  * Settings are {@link com.squareup.okhttp.internal.spdy.SpdyConnection connection} scoped.
  */
 final class Settings {
+  /**
+   * From the SPDY/3 and HTTP/2 specs, 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;
+
   /** Peer request to clear durable settings. */
   static final int FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS = 0x1;
 
@@ -171,11 +177,9 @@ final class Settings {
     return (bit & set) != 0 ? values[DOWNLOAD_RETRANS_RATE] : defaultValue;
   }
 
-  // TODO: honor this setting in http/2.
-  /** Returns -1 if unset. */
-  int getInitialWindowSize() {
+  int getInitialWindowSize(int defaultValue) {
     int bit = 1 << INITIAL_WINDOW_SIZE;
-    return (bit & set) != 0 ? values[INITIAL_WINDOW_SIZE] : -1;
+    return (bit & set) != 0 ? values[INITIAL_WINDOW_SIZE] : defaultValue;
   }
 
   /** spdy/3 only. */
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 1ab4ab531..5c10db157 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
@@ -38,6 +38,8 @@ import java.util.concurrent.SynchronousQueue;
 import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 
+import static com.squareup.okhttp.internal.spdy.Settings.DEFAULT_INITIAL_WINDOW_SIZE;
+
 /**
  * A socket connection to a remote peer. A connection hosts streams which can
  * send and receive data.
@@ -89,8 +91,6 @@ public final class SpdyConnection implements Closeable {
   private final PushObserver pushObserver;
   private int nextPingId;
 
-  static final int INITIAL_WINDOW_SIZE = 65535;
-
   /**
    * The total number of bytes consumed by the application, but not yet
    * acknowledged by sending a {@code WINDOW_UPDATE} frame on this connection.
@@ -107,15 +107,12 @@ public final class SpdyConnection implements Closeable {
 
   /** Settings we communicate to the peer. */
   // TODO: Do we want to dynamically adjust settings, or KISS and only set once?
-  final Settings okHttpSettings = new Settings()
-      .set(Settings.INITIAL_WINDOW_SIZE, 0, INITIAL_WINDOW_SIZE);
-      // TODO: implement stream limit
+  final Settings okHttpSettings = new Settings();
       // okHttpSettings.set(Settings.MAX_CONCURRENT_STREAMS, 0, max);
 
   /** Settings we receive from the peer. */
   // TODO: MWS will need to guard on this setting before attempting to push.
-  final Settings peerSettings = new Settings()
-      .set(Settings.INITIAL_WINDOW_SIZE, 0, INITIAL_WINDOW_SIZE);
+  final Settings peerSettings = new Settings();
 
   private boolean receivedInitialPeerSettings = false;
   final FrameReader frameReader;
@@ -151,7 +148,7 @@ public final class SpdyConnection implements Closeable {
     } else {
       throw new AssertionError(protocol);
     }
-    bytesLeftInWriteWindow = peerSettings.getInitialWindowSize();
+    bytesLeftInWriteWindow = peerSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE);
     frameReader = variant.newReader(builder.source, client);
     frameWriter = variant.newWriter(builder.sink, client);
     maxFrameSize = variant.maxFrameSize();
@@ -657,16 +654,13 @@ public final class SpdyConnection implements Closeable {
       long delta = 0;
       SpdyStream[] streamsToNotify = null;
       synchronized (SpdyConnection.this) {
-        int priorWriteWindowSize = peerSettings.getInitialWindowSize();
-        if (clearPrevious) {
-          peerSettings.clear();
-        } else {
-          peerSettings.merge(newSettings);
-        }
+        int priorWriteWindowSize = peerSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE);
+        if (clearPrevious) peerSettings.clear();
+        peerSettings.merge(newSettings);
         if (getProtocol() == Protocol.HTTP_2) {
           ackSettingsLater();
         }
-        int peerInitialWindowSize = peerSettings.getInitialWindowSize();
+        int peerInitialWindowSize = peerSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE);
         if (peerInitialWindowSize != -1 && peerInitialWindowSize != priorWriteWindowSize) {
           delta = peerInitialWindowSize - priorWriteWindowSize;
           if (!receivedInitialPeerSettings) {
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 d0d2972aa..58d116fec 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
@@ -28,6 +28,8 @@ import java.net.SocketTimeoutException;
 import java.util.ArrayList;
 import java.util.List;
 
+import static com.squareup.okhttp.internal.spdy.Settings.DEFAULT_INITIAL_WINDOW_SIZE;
+
 /** A logical bidirectional stream. */
 public final class SpdyStream {
   // Internal state is guarded by this. No long-running or potentially
@@ -76,8 +78,10 @@ public final class SpdyStream {
     if (requestHeaders == null) throw new NullPointerException("requestHeaders == null");
     this.id = id;
     this.connection = connection;
-    this.bytesLeftInWriteWindow = connection.peerSettings.getInitialWindowSize();
-    this.source = new SpdyDataSource(connection.okHttpSettings.getInitialWindowSize());
+    this.bytesLeftInWriteWindow =
+        connection.peerSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE);
+    this.source = new SpdyDataSource(
+        connection.okHttpSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE));
     this.sink = new SpdyDataSink();
     this.source.finished = inFinished;
     this.sink.finished = outFinished;
@@ -370,7 +374,8 @@ public final class SpdyStream {
 
         // Flow control: notify the peer that we're ready for more data!
         unacknowledgedBytesRead += read;
-        if (unacknowledgedBytesRead >= connection.peerSettings.getInitialWindowSize() / 2) {
+        if (unacknowledgedBytesRead
+            >= connection.peerSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE) / 2) {
           connection.writeWindowUpdateLater(id, unacknowledgedBytesRead);
           unacknowledgedBytesRead = 0;
         }
@@ -380,7 +385,7 @@ public final class SpdyStream {
       synchronized (connection) { // Multiple application threads may hit this section.
         connection.unacknowledgedBytesRead += read;
         if (connection.unacknowledgedBytesRead
-            >= connection.peerSettings.getInitialWindowSize() / 2) {
+            >= connection.peerSettings.getInitialWindowSize(DEFAULT_INITIAL_WINDOW_SIZE) / 2) {
           connection.writeWindowUpdateLater(0, connection.unacknowledgedBytesRead);
           connection.unacknowledgedBytesRead = 0;
         }