all: Add suppressions for GuardedBy violations (grpc#7291)

This supports releasing a new version of GuardedBy which finds more mistakes than it used to. Filed grpc#6578 to try to clean up the suppressions. Co-authored-by: Graeme Morgan <mail@graememorgan.uk>
ejona86 · Aug 17, 2020 · bb47680 · bb47680
1 parent 62e8655
commit bb47680
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 0 deletions.
diff --git a/core/src/main/java/io/grpc/internal/RetriableStream.java b/core/src/main/java/io/grpc/internal/RetriableStream.java
@@ -121,6 +121,7 @@ abstract class RetriableStream<ReqT> implements ClientStream {
     this.throttle = throttle;
   }
 
+  @SuppressWarnings("GuardedBy")
   @Nullable // null if already committed
   @CheckReturnValue
   private Runnable commit(final Substream winningSubstream) {
@@ -138,6 +139,8 @@ private Runnable commit(final Substream winningSubstream) {
 
       final Future<?> retryFuture;
       if (scheduledRetry != null) {
+        // TODO(b/145386688): This access should be guarded by 'this.scheduledRetry.lock'; instead
+        // found: 'this.lock'
         retryFuture = scheduledRetry.markCancelled();
         scheduledRetry = null;
       } else {
@@ -146,6 +149,8 @@ private Runnable commit(final Substream winningSubstream) {
       // cancel the scheduled hedging if it is scheduled prior to the commitment
       final Future<?> hedgingFuture;
       if (scheduledHedging != null) {
+        // TODO(b/145386688): This access should be guarded by 'this.scheduledHedging.lock'; instead
+        // found: 'this.lock'
         hedgingFuture = scheduledHedging.markCancelled();
         scheduledHedging = null;
       } else {
@@ -338,6 +343,7 @@ public void runWith(Substream substream) {
     drain(substream);
   }
 
+  @SuppressWarnings("GuardedBy")
   private void pushbackHedging(@Nullable Integer delayMillis) {
     if (delayMillis == null) {
       return;
@@ -356,6 +362,8 @@ private void pushbackHedging(@Nullable Integer delayMillis) {
         return;
       }
 
+      // TODO(b/145386688): This access should be guarded by 'this.scheduledHedging.lock'; instead
+      // found: 'this.lock'
       futureToBeCancelled = scheduledHedging.markCancelled();
       scheduledHedging = future = new FutureCanceller(lock);
     }
@@ -381,6 +389,7 @@ private final class HedgingRunnable implements Runnable {
     public void run() {
       callExecutor.execute(
           new Runnable() {
+            @SuppressWarnings("GuardedBy")
             @Override
             public void run() {
               // It's safe to read state.hedgingAttemptCount here.
@@ -392,6 +401,9 @@ public void run() {
               FutureCanceller future = null;
 
               synchronized (lock) {
+                // TODO(b/145386688): This access should be guarded by
+                // 'HedgingRunnable.this.scheduledHedgingRef.lock'; instead found:
+                // 'RetriableStream.this.lock'
                 if (scheduledHedgingRef.isCancelled()) {
                   cancelled = true;
                 } else {
@@ -695,10 +707,13 @@ private boolean hasPotentialHedging(State state) {
         && !state.hedgingFrozen;
   }
 
+  @SuppressWarnings("GuardedBy")
   private void freezeHedging() {
     Future<?> futureToBeCancelled = null;
     synchronized (lock) {
       if (scheduledHedging != null) {
+        // TODO(b/145386688): This access should be guarded by 'this.scheduledHedging.lock'; instead
+        // found: 'this.lock'
         futureToBeCancelled = scheduledHedging.markCancelled();
         scheduledHedging = null;
       }

diff --git a/cronet/src/main/java/io/grpc/cronet/CronetClientTransport.java b/cronet/src/main/java/io/grpc/cronet/CronetClientTransport.java
@@ -142,9 +142,12 @@ public void run() {
     return new StartCallback().clientStream;
   }
 
+  @SuppressWarnings("GuardedBy")
   @GuardedBy("lock")
   private void startStream(CronetClientStream stream) {
     streams.add(stream);
+    // TODO(b/145386688): This access should be guarded by 'stream.transportState().lock'; instead
+    // found: 'this.lock'
     stream.transportState().start(streamFactory);
   }
 

diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientStream.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientStream.java
@@ -255,10 +255,13 @@ public TransportState(
       tag = PerfMark.createTag(methodName);
     }
 
+    @SuppressWarnings("GuardedBy")
     @GuardedBy("lock")
     public void start(int streamId) {
       checkState(id == ABSENT_ID, "the stream has been started with id %s", streamId);
       id = streamId;
+      // TODO(b/145386688): This access should be guarded by 'OkHttpClientStream.this.state.lock';
+      // instead found: 'this.lock'
       state.onStreamAllocated();
 
       if (canStart) {
@@ -364,6 +367,7 @@ private void onEndOfStream() {
       }
     }
 
+    @SuppressWarnings("GuardedBy")
     @GuardedBy("lock")
     private void cancel(Status reason, boolean stopDelivery, Metadata trailers) {
       if (cancelSent) {
@@ -372,6 +376,8 @@ private void cancel(Status reason, boolean stopDelivery, Metadata trailers) {
       cancelSent = true;
       if (canStart) {
         // stream is pending.
+        // TODO(b/145386688): This access should be guarded by 'this.transport.lock'; instead found:
+        // 'this.lock'
         transport.removePendingStream(OkHttpClientStream.this);
         // release holding data, so they can be GCed or returned to pool earlier.
         requestHeaders = null;
@@ -405,6 +411,7 @@ private void sendBuffer(Buffer buffer, boolean endOfStream, boolean flush) {
       }
     }
 
+    @SuppressWarnings("GuardedBy")
     @GuardedBy("lock")
     private void streamReady(Metadata metadata, String path) {
       requestHeaders =
@@ -415,6 +422,8 @@ private void streamReady(Metadata metadata, String path) {
               userAgent,
               useGet,
               transport.isUsingPlaintext());
+      // TODO(b/145386688): This access should be guarded by 'this.transport.lock'; instead found:
+      // 'this.lock'
       transport.streamReadyToStart(OkHttpClientStream.this);
     }
 

diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java
@@ -419,12 +419,15 @@ void streamReadyToStart(OkHttpClientStream clientStream) {
     }
   }
 
+  @SuppressWarnings("GuardedBy")
   @GuardedBy("lock")
   private void startStream(OkHttpClientStream stream) {
     Preconditions.checkState(
         stream.id() == OkHttpClientStream.ABSENT_ID, "StreamId already assigned");
     streams.put(nextStreamId, stream);
     setInUse(stream);
+    // TODO(b/145386688): This access should be guarded by 'stream.transportState().lock'; instead
+    // found: 'this.lock'
     stream.transportState().start(nextStreamId);
     // For unary and server streaming, there will be a data frame soon, no need to flush the header.
     if ((stream.getType() != MethodType.UNARY && stream.getType() != MethodType.SERVER_STREAMING)
@@ -1106,6 +1109,7 @@ public void run() {
     /**
      * Handle an HTTP2 DATA frame.
      */
+    @SuppressWarnings("GuardedBy")
     @Override
     public void data(boolean inFinished, int streamId, BufferedSource in, int length)
         throws IOException {
@@ -1131,6 +1135,8 @@ public void data(boolean inFinished, int streamId, BufferedSource in, int length
         PerfMark.event("OkHttpClientTransport$ClientFrameHandler.data",
             stream.transportState().tag());
         synchronized (lock) {
+          // TODO(b/145386688): This access should be guarded by 'stream.transportState().lock';
+          // instead found: 'OkHttpClientTransport.this.lock'
           stream.transportState().transportDataReceived(buf, inFinished);
         }
       }
@@ -1148,6 +1154,7 @@ public void data(boolean inFinished, int streamId, BufferedSource in, int length
     /**
      * Handle HTTP2 HEADER and CONTINUATION frames.
      */
+    @SuppressWarnings("GuardedBy")
     @Override
     public void headers(boolean outFinished,
         boolean inFinished,
@@ -1181,6 +1188,8 @@ public void headers(boolean outFinished,
           if (failedStatus == null) {
             PerfMark.event("OkHttpClientTransport$ClientFrameHandler.headers",
                 stream.transportState().tag());
+            // TODO(b/145386688): This access should be guarded by 'stream.transportState().lock';
+            // instead found: 'OkHttpClientTransport.this.lock'
             stream.transportState().transportHeadersReceived(headerBlock, inFinished);
           } else {
             if (!inFinished) {