googleapis / gax-java

This library has moved to https://github.com/googleapis/sdk-platform-java/tree/main/gax-java.
https://github.com/googleapis/gapic-generator-java/tree/main/gax-java
BSD 3-Clause "New" or "Revised" License
162 stars 119 forks source link

fix: Handle cancel() in ReleasingClientCall and rethrow the exception in start() #1945

Closed mutianf closed 1 year ago

mutianf commented 1 year ago

We got a customer issue and the stacktrace didn't show us where the call was cancelled (b/263968439)

When trying to reproduce this error, we noticed some inconsistent behaviors when we cancel the stream in onStart()

client.readRowsCallable().call(Query.create("t"), new ResponseObserver<Row>() {
    @Override
    public void onStart(StreamController streamController) {
        streamController.cancel();
    }
    @Override
    public void onResponse(Row row) {
    }
    @Override
    public void onError(Throwable throwable) {
    }
    @Override
    public void onComplete() {
    }
});

When the above code is called, we got exception:

Caused by: java.util.concurrent.CancellationException: User cancelled stream
    at com.google.api.gax.rpc.ServerStreamingAttemptCallable.onCancel(ServerStreamingAttemptCallable.java:309)

However, if we make another call before it:

Iterator<Row> stream = client.readRowsCallable().call(Query.create("t")).iterator();
client.readRowsCallable().call(Query.create("t"), new ResponseObserver<Row>() {
    @Override
    public void onStart(StreamController streamController) {
        streamController.cancel();
    }
   ...
}

We got exception:

java.lang.IllegalStateException: Not started
    at com.google.common.base.Preconditions.checkState(Preconditions.java:502)
    at io.grpc.internal.ClientCallImpl.sendMessageInternal(ClientCallImpl.java:511)
    at io.grpc.internal.ClientCallImpl.sendMessage(ClientCallImpl.java:504)

Which is very difficult to debug.

I think there are 2 problems in the ReleasingClientCall:

  1. It's not tracking cancellation status: After the call is cancelled, it continued to go into start() logic and eventually will call ClientCallImpl#startInternal https://github.com/grpc/grpc-java/blob/v1.51.1/core/src/main/java/io/grpc/internal/ClientCallImpl.java#L199, where the cancelCalled will be true and the state check will fail.
  2. It's not rethrowing the error in onStart(): The IllegalStateException thrown from ClientCallImpl#startInternal will be caught, GrpcDirectStreamController will move on to ClientCallImpl#sendMessage and eventually fail when checking if the stream is started https://github.com/grpc/grpc-java/blob/v1.51.1/core/src/main/java/io/grpc/internal/ClientCallImpl.java#L514.
sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

45.5% 45.5% Coverage
0.0% 0.0% Duplication

blakeli0 commented 1 year ago

@mutianf This repo has recently been migrated to here in gapic-generator-java, do you mind closing this PR and recreating in gapic-generator-java repo? Sorry for the late notice as we are still in the middle of migration.