all: Add suppressions for GuardedBy violations

This supports releasing a new version of GuardedBy which finds more mistakes than it used to.

Filed #6578 to try to clean up the suppressions.
This commit is contained in:
Graeme Morgan 2019-12-30 18:17:05 +00:00 committed by ZHANG Dapeng
parent 19805233ef
commit d3c77f2d87
4 changed files with 36 additions and 0 deletions

View File

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

View File

@ -149,9 +149,12 @@ class CronetClientTransport implements ConnectionClientTransport {
return new StartCallback().clientStream; return new StartCallback().clientStream;
} }
@SuppressWarnings("GuardedBy")
@GuardedBy("lock") @GuardedBy("lock")
private void startStream(CronetClientStream stream) { private void startStream(CronetClientStream stream) {
streams.add(stream); streams.add(stream);
// TODO(b/145386688): This access should be guarded by 'stream.transportState().lock'; instead
// found: 'this.lock'
stream.transportState().start(streamFactory); stream.transportState().start(streamFactory);
} }

View File

@ -256,10 +256,13 @@ class OkHttpClientStream extends AbstractClientStream {
tag = PerfMark.createTag(methodName); tag = PerfMark.createTag(methodName);
} }
@SuppressWarnings("GuardedBy")
@GuardedBy("lock") @GuardedBy("lock")
public void start(int streamId) { public void start(int streamId) {
checkState(id == ABSENT_ID, "the stream has been started with id %s", streamId); checkState(id == ABSENT_ID, "the stream has been started with id %s", streamId);
id = streamId; id = streamId;
// TODO(b/145386688): This access should be guarded by 'OkHttpClientStream.this.state.lock';
// instead found: 'this.lock'
state.onStreamAllocated(); state.onStreamAllocated();
if (canStart) { if (canStart) {
@ -365,6 +368,7 @@ class OkHttpClientStream extends AbstractClientStream {
} }
} }
@SuppressWarnings("GuardedBy")
@GuardedBy("lock") @GuardedBy("lock")
private void cancel(Status reason, boolean stopDelivery, Metadata trailers) { private void cancel(Status reason, boolean stopDelivery, Metadata trailers) {
if (cancelSent) { if (cancelSent) {
@ -373,6 +377,8 @@ class OkHttpClientStream extends AbstractClientStream {
cancelSent = true; cancelSent = true;
if (canStart) { if (canStart) {
// stream is pending. // stream is pending.
// TODO(b/145386688): This access should be guarded by 'this.transport.lock'; instead found:
// 'this.lock'
transport.removePendingStream(OkHttpClientStream.this); transport.removePendingStream(OkHttpClientStream.this);
// release holding data, so they can be GCed or returned to pool earlier. // release holding data, so they can be GCed or returned to pool earlier.
requestHeaders = null; requestHeaders = null;
@ -406,6 +412,7 @@ class OkHttpClientStream extends AbstractClientStream {
} }
} }
@SuppressWarnings("GuardedBy")
@GuardedBy("lock") @GuardedBy("lock")
private void streamReady(Metadata metadata, String path) { private void streamReady(Metadata metadata, String path) {
requestHeaders = requestHeaders =
@ -416,6 +423,8 @@ class OkHttpClientStream extends AbstractClientStream {
userAgent, userAgent,
useGet, useGet,
transport.isUsingPlaintext()); transport.isUsingPlaintext());
// TODO(b/145386688): This access should be guarded by 'this.transport.lock'; instead found:
// 'this.lock'
transport.streamReadyToStart(OkHttpClientStream.this); transport.streamReadyToStart(OkHttpClientStream.this);
} }

View File

@ -424,12 +424,15 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
} }
} }
@SuppressWarnings("GuardedBy")
@GuardedBy("lock") @GuardedBy("lock")
private void startStream(OkHttpClientStream stream) { private void startStream(OkHttpClientStream stream) {
Preconditions.checkState( Preconditions.checkState(
stream.id() == OkHttpClientStream.ABSENT_ID, "StreamId already assigned"); stream.id() == OkHttpClientStream.ABSENT_ID, "StreamId already assigned");
streams.put(nextStreamId, stream); streams.put(nextStreamId, stream);
setInUse(stream); setInUse(stream);
// TODO(b/145386688): This access should be guarded by 'stream.transportState().lock'; instead
// found: 'this.lock'
stream.transportState().start(nextStreamId); stream.transportState().start(nextStreamId);
// For unary and server streaming, there will be a data frame soon, no need to flush the header. // 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) if ((stream.getType() != MethodType.UNARY && stream.getType() != MethodType.SERVER_STREAMING)
@ -1111,6 +1114,7 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
/** /**
* Handle an HTTP2 DATA frame. * Handle an HTTP2 DATA frame.
*/ */
@SuppressWarnings("GuardedBy")
@Override @Override
public void data(boolean inFinished, int streamId, BufferedSource in, int length) public void data(boolean inFinished, int streamId, BufferedSource in, int length)
throws IOException { throws IOException {
@ -1136,6 +1140,8 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
PerfMark.event("OkHttpClientTransport$ClientFrameHandler.data", PerfMark.event("OkHttpClientTransport$ClientFrameHandler.data",
stream.transportState().tag()); stream.transportState().tag());
synchronized (lock) { synchronized (lock) {
// TODO(b/145386688): This access should be guarded by 'stream.transportState().lock';
// instead found: 'OkHttpClientTransport.this.lock'
stream.transportState().transportDataReceived(buf, inFinished); stream.transportState().transportDataReceived(buf, inFinished);
} }
} }
@ -1153,6 +1159,7 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
/** /**
* Handle HTTP2 HEADER and CONTINUATION frames. * Handle HTTP2 HEADER and CONTINUATION frames.
*/ */
@SuppressWarnings("GuardedBy")
@Override @Override
public void headers(boolean outFinished, public void headers(boolean outFinished,
boolean inFinished, boolean inFinished,
@ -1186,6 +1193,8 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
if (failedStatus == null) { if (failedStatus == null) {
PerfMark.event("OkHttpClientTransport$ClientFrameHandler.headers", PerfMark.event("OkHttpClientTransport$ClientFrameHandler.headers",
stream.transportState().tag()); 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); stream.transportState().transportHeadersReceived(headerBlock, inFinished);
} else { } else {
if (!inFinished) { if (!inFinished) {