grpc / grpc-java

The Java gRPC implementation. HTTP/2 based RPC
https://grpc.io/docs/languages/java/
Apache License 2.0
11.48k stars 3.86k forks source link

CallCredentials should respect the Deadline #4929

Open igorbernstein2 opened 6 years ago

igorbernstein2 commented 6 years ago

Please answer these questions before submitting your issue.

What version of gRPC are you using?

1.15.1

What did you expect to see?

An RPC should never outlast a deadline set in either CallOptions or `grpc.

What I actually saw?

CallCredentials does not respect deadlines, so a ClientCall cannot guarantee that the RPC will finish within a set deadline (for example if an OAuth token needs to be refreshed). Furthermore, the current CallCredential api makes it impossible for CallCredentials to access the deadline set in CallOptions.

zhangkun83 commented 6 years ago

Even though CallCredentials currently doesn't observe deadline, I believe ClientCall still guarantees the deadline. It will be cancelled on deadline even if CallCredentials is still working. (Please correct me if you find out otherwise). CallCredentials could observe deadline and stop the work since it's no longer needed.

A related issue: if CallCredentials eventually succeeds after the ClientCall has been cancelled by deadline, a new "real" stream will be passed to and discarded by DelayedStream.setStream(). This is another case where #1537 can cause harm. We should probably revive that issue (cc @ejona86)

igorbernstein2 commented 6 years ago

Hmm, it doesn't seem like the behavior matches. The call seems to block until the call creds are applied. Here is a same program I used to verify:

public class Main {
  public static void main(String[] args) throws IOException, InterruptedException {
    Server server = NettyServerBuilder.forPort(1234)
        .addService(new BigtableImplBase() {})
        .build();

    server.start();

    ManagedChannel channel = NettyChannelBuilder.forAddress("localhost", 1234)
        .usePlaintext()
        .build();

    BigtableBlockingStub stub = BigtableGrpc.newBlockingStub(channel)
        .withCallCredentials(new HangingCallCredentials());

    System.out.println("starting rpc");
    try {
      Iterator<ReadRowsResponse> iter = stub
          .withDeadline(Deadline.after(10, TimeUnit.SECONDS))
          .readRows(ReadRowsRequest.getDefaultInstance());

      iter.next();
    } catch (Exception e) {
      System.out.println("done rpc");
      e.printStackTrace();
    }

    Thread.sleep(60_000);
  }

  static class HangingCallCredentials implements CallCredentials {

    @Override
    public void applyRequestMetadata(MethodDescriptor<?, ?> method, Attributes attrs,
        Executor appExecutor, MetadataApplier applier) {

      appExecutor.execute(() -> {
        try {
          Thread.sleep(20_000);
        } catch (InterruptedException e) {
          e.printStackTrace();
        }
        System.out.println("done sleeping");
        applier.apply(new Metadata());
      });
    }

    @Override
    public void thisUsesUnstableApi() {

    }
  }
}

Output:

starting rpc
done sleeping
done rpc
io.grpc.StatusRuntimeException: DEADLINE_EXCEEDED: deadline exceeded after 9965054578ns
    at io.grpc.Status.asRuntimeException(Status.java:526)
    at io.grpc.stub.ClientCalls$BlockingResponseStream.hasNext(ClientCalls.java:563)
    at io.grpc.stub.ClientCalls$BlockingResponseStream.next(ClientCalls.java:570)
    at Main.main(Main.java:41)
zhangkun83 commented 6 years ago

As @carl-mastrangelo pointed out, the effective deadline that ClientCallImpl uses is the combination of the deadline from CallOptions and the current Context (see ClientCallImpl.effectiveDeadline()), and that is the deadline we should pass in RequestInfo.

dapengzhang0 commented 5 years ago

Modified milestone to 'Next': Need assignment.