From 054cb49b49fb65d4383a6e4a62419df4aef0813c Mon Sep 17 00:00:00 2001
From: Eric Anderson <ejona@google.com>
Date: Thu, 7 Apr 2022 13:30:25 -0700
Subject: [PATCH] okhttp: Remove RPCs-before-ready tests

In the olden days, before LB policies, transports had to accept RPCs as
soon as they were created. This hasn't been true for a very long time,
so remove the tests.

Since a978c9ed we're using real, legit code flows in the tests. This
allowed TSAN to discover that `attributes` is racy when read when
creating a new stream before the transport is ready. We could use a lock
or volatile, but the value of the attributes would still be incorrect
for any RPCs that are created before the transport is ready.

Since there's now only one test that delays the connection, I inline the
support code.
---
 .../okhttp/OkHttpClientTransportTest.java     | 97 ++-----------------
 1 file changed, 8 insertions(+), 89 deletions(-)

diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
index 1c676f2ac86..ec4f7b37ca5 100644
--- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
+++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
@@ -45,7 +45,6 @@
 import static org.mockito.Mockito.timeout;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
-import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;
@@ -171,7 +170,6 @@ public class OkHttpClientTransportTest {
   private ExecutorService executor = Executors.newCachedThreadPool();
   private long nanoTime; // backs a ticker, for testing ping round-trip time measurement
   private SettableFuture<Void> connectedFuture;
-  private DelayConnectedCallback delayConnectedCallback;
   private Runnable tooManyPingsRunnable = new Runnable() {
     @Override public void run() {
       throw new AssertionError();
@@ -204,15 +202,6 @@ private void initTransport(int startId) throws Exception {
     startTransport(startId, null, true, null);
   }
 
-  private void initTransportAndDelayConnected() throws Exception {
-    delayConnectedCallback = new DelayConnectedCallback();
-    startTransport(
-        DEFAULT_START_STREAM_ID,
-        delayConnectedCallback,
-        false,
-        null);
-  }
-
   private void startTransport(int startId, @Nullable Runnable connectingCallback,
       boolean waitingForConnected, String userAgent)
       throws Exception {
@@ -1681,70 +1670,17 @@ public void ping_failsIfTransportFails() throws Exception {
     shutdownAndVerify();
   }
 
-  @Test
-  public void writeBeforeConnected() throws Exception {
-    initTransportAndDelayConnected();
-    reset(frameWriter);
-    final String message = "Hello Server";
-    MockStreamListener listener = new MockStreamListener();
-    OkHttpClientStream stream =
-        clientTransport.newStream(method, new Metadata(), CallOptions.DEFAULT, tracers);
-    stream.start(listener);
-    InputStream input = new ByteArrayInputStream(message.getBytes(UTF_8));
-    stream.writeMessage(input);
-    stream.flush();
-    // The message should be queued.
-    verifyNoMoreInteractions(frameWriter);
-
-    allowTransportConnected();
-
-    // The queued message should be sent out.
-    verify(frameWriter, timeout(TIME_OUT_MS))
-        .data(eq(false), eq(3), any(Buffer.class), eq(12 + HEADER_LENGTH));
-    Buffer sentFrame = capturedBuffer.poll();
-    assertEquals(createMessageFrame(message), sentFrame);
-    stream.cancel(Status.CANCELLED);
-    shutdownAndVerify();
-  }
-
-  @Test
-  public void cancelBeforeConnected() throws Exception {
-    initTransportAndDelayConnected();
-    reset(frameWriter);
-    final String message = "Hello Server";
-    MockStreamListener listener = new MockStreamListener();
-    OkHttpClientStream stream =
-        clientTransport.newStream(method, new Metadata(), CallOptions.DEFAULT, tracers);
-    stream.start(listener);
-    InputStream input = new ByteArrayInputStream(message.getBytes(UTF_8));
-    stream.writeMessage(input);
-    stream.flush();
-    stream.cancel(Status.CANCELLED);
-    verifyNoMoreInteractions(frameWriter);
-
-    allowTransportConnected();
-    verifyNoMoreInteractions(frameWriter);
-    shutdownAndVerify();
-  }
-
   @Test
   public void shutdownDuringConnecting() throws Exception {
-    initTransportAndDelayConnected();
-    MockStreamListener listener = new MockStreamListener();
-    OkHttpClientStream stream =
-        clientTransport.newStream(method, new Metadata(), CallOptions.DEFAULT, tracers);
-    stream.start(listener);
+    SettableFuture<Void> delayed = SettableFuture.create();
+    Runnable connectingCallback = () -> Futures.getUnchecked(delayed);
+    startTransport(
+        DEFAULT_START_STREAM_ID,
+        connectingCallback,
+        false,
+        null);
     clientTransport.shutdown(SHUTDOWN_REASON);
-    allowTransportConnected();
-
-    // The new stream should be failed, but not the pending stream.
-    assertNewStreamFail();
-    verify(frameWriter, timeout(TIME_OUT_MS))
-        .synStream(anyBoolean(), anyBoolean(), eq(3), anyInt(), anyListHeader());
-    assertEquals(1, activeStreamCount());
-    stream.cancel(Status.CANCELLED);
-    listener.waitUntilStreamClosed();
-    assertEquals(Status.CANCELLED.getCode(), listener.status.getCode());
+    delayed.set(null);
     shutdownAndVerify();
   }
 
@@ -2375,10 +2311,6 @@ public void onFailure(Throwable cause) {
     }
   }
 
-  private void allowTransportConnected() {
-    delayConnectedCallback.allowConnected();
-  }
-
   private void shutdownAndVerify() {
     clientTransport.shutdown(SHUTDOWN_REASON);
     assertEquals(0, activeStreamCount());
@@ -2390,19 +2322,6 @@ private void shutdownAndVerify() {
     frameReader.assertClosed();
   }
 
-  private static class DelayConnectedCallback implements Runnable {
-    SettableFuture<Void> delayed = SettableFuture.create();
-
-    @Override
-    public void run() {
-      Futures.getUnchecked(delayed);
-    }
-
-    void allowConnected() {
-      delayed.set(null);
-    }
-  }
-
   private static TransportStats getTransportStats(InternalInstrumented<SocketStats> obj)
       throws ExecutionException, InterruptedException {
     return obj.getStats().get().data;