Acknowledge and apply inbound settings atomically

swankjesse · swankjesse · commit a7248061a301 · 2019-09-09T18:06:50.000-04:00
Closes: #5422 Unfortunately testing this is awkward because it's racy. I did run a stress test that used to reproduce the problem, and now it doesn't, so I am satisfied.
diff --git a/okhttp-tests/src/test/java/okhttp3/internal/http2/Http2ConnectionTest.java b/okhttp-tests/src/test/java/okhttp3/internal/http2/Http2ConnectionTest.java
@@ -946,7 +946,7 @@ public final class Http2ConnectionTest {
     // fake a settings frame with clear flag set.
     Settings settings2 = new Settings();
     settings2.set(MAX_CONCURRENT_STREAMS, 60000);
-    connection.readerRunnable.settings(true, settings2);
+    connection.readerRunnable.applyAndAckSettings(true, settings2);
 
     synchronized (connection) {
       assertThat(connection.peerSettings.getHeaderTableSize()).isEqualTo(-1);
diff --git a/okhttp/src/main/java/okhttp3/internal/http2/Http2Connection.java b/okhttp/src/main/java/okhttp3/internal/http2/Http2Connection.java
@@ -129,7 +129,6 @@ public final class Http2Connection implements Closeable {
   // TODO: MWS will need to guard on this setting before attempting to push.
   final Settings peerSettings = new Settings();
 
-  boolean receivedInitialPeerSettings = false;
   final Socket socket;
   final Http2Writer writer;
 
@@ -690,53 +689,52 @@ class ReaderRunnable extends NamedRunnable implements Http2Reader.Handler {
       }
     }
 
-    @Override public void settings(boolean clearPrevious, Settings newSettings) {
+    @Override public void settings(boolean clearPrevious, Settings settings) {
+      try {
+        writerExecutor.execute(new NamedRunnable("OkHttp %s ACK Settings", connectionName) {
+          @Override public void execute() {
+            applyAndAckSettings(clearPrevious, settings);
+          }
+        });
+      } catch (RejectedExecutionException ignored) {
+        // This connection has been closed.
+      }
+    }
+
+    void applyAndAckSettings(boolean clearPrevious, Settings settings) {
       long delta = 0;
       Http2Stream[] streamsToNotify = null;
-      synchronized (Http2Connection.this) {
-        int priorWriteWindowSize = peerSettings.getInitialWindowSize();
-        if (clearPrevious) peerSettings.clear();
-        peerSettings.merge(newSettings);
-        applyAndAckSettings(newSettings);
-        int peerInitialWindowSize = peerSettings.getInitialWindowSize();
-        if (peerInitialWindowSize != -1 && peerInitialWindowSize != priorWriteWindowSize) {
-          delta = peerInitialWindowSize - priorWriteWindowSize;
-          if (!receivedInitialPeerSettings) {
-            receivedInitialPeerSettings = true;
-          }
-          if (!streams.isEmpty()) {
-            streamsToNotify = streams.values().toArray(new Http2Stream[streams.size()]);
+      synchronized (writer) {
+        synchronized (Http2Connection.this) {
+          int priorWriteWindowSize = peerSettings.getInitialWindowSize();
+          if (clearPrevious) peerSettings.clear();
+          peerSettings.merge(settings);
+          int peerInitialWindowSize = peerSettings.getInitialWindowSize();
+          if (peerInitialWindowSize != -1 && peerInitialWindowSize != priorWriteWindowSize) {
+            delta = peerInitialWindowSize - priorWriteWindowSize;
+            streamsToNotify = !streams.isEmpty()
+                ? streams.values().toArray(new Http2Stream[streams.size()])
+                : null;
           }
         }
-        listenerExecutor.execute(new NamedRunnable("OkHttp %s settings", connectionName) {
-          @Override public void execute() {
-            listener.onSettings(Http2Connection.this);
-          }
-        });
+        try {
+          writer.applyAndAckSettings(peerSettings);
+        } catch (IOException e) {
+          failConnection(e);
+        }
       }
-      if (streamsToNotify != null && delta != 0) {
+      if (streamsToNotify != null) {
         for (Http2Stream stream : streamsToNotify) {
           synchronized (stream) {
             stream.addBytesToWriteWindow(delta);
           }
         }
       }
-    }
-
-    private void applyAndAckSettings(final Settings peerSettings) {
-      try {
-        writerExecutor.execute(new NamedRunnable("OkHttp %s ACK Settings", connectionName) {
-          @Override public void execute() {
-            try {
-              writer.applyAndAckSettings(peerSettings);
-            } catch (IOException e) {
-              failConnection(e);
-            }
-          }
-        });
-      } catch (RejectedExecutionException ignored) {
-        // This connection has been closed.
-      }
+      listenerExecutor.execute(new NamedRunnable("OkHttp %s settings", connectionName) {
+        @Override public void execute() {
+          listener.onSettings(Http2Connection.this);
+        }
+      });
     }
 
     @Override public void ackSettings() {
