Skip to content

Add suppressions for GuardedBy violations #6566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Dec 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions core/src/main/java/io/grpc/internal/RetriableStream.java
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -338,6 +343,7 @@ public void runWith(Substream substream) {
drain(substream);
}

@SuppressWarnings("GuardedBy")
private void pushbackHedging(@Nullable Integer delayMillis) {
if (delayMillis == null) {
return;
Expand All @@ -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);
}
Expand All @@ -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.
Expand All @@ -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 {
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,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);
}

Expand Down
9 changes: 9 additions & 0 deletions okhttp/src/main/java/io/grpc/okhttp/OkHttpClientStream.java
Original file line number Diff line number Diff line change
Expand Up @@ -256,10 +256,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) {
Expand Down Expand Up @@ -365,6 +368,7 @@ private void onEndOfStream() {
}
}

@SuppressWarnings("GuardedBy")
@GuardedBy("lock")
private void cancel(Status reason, boolean stopDelivery, Metadata trailers) {
if (cancelSent) {
Expand All @@ -373,6 +377,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;
Expand Down Expand Up @@ -406,6 +412,7 @@ private void sendBuffer(Buffer buffer, boolean endOfStream, boolean flush) {
}
}

@SuppressWarnings("GuardedBy")
@GuardedBy("lock")
private void streamReady(Metadata metadata, String path) {
requestHeaders =
Expand All @@ -416,6 +423,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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -424,12 +424,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)
Expand Down Expand Up @@ -1111,6 +1114,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 {
Expand All @@ -1136,6 +1140,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);
}
}
Expand All @@ -1153,6 +1159,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,
Expand Down Expand Up @@ -1186,6 +1193,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) {
Expand Down