Skip to content

CallCredentials should respect the Deadline #4929

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

Open
igorbernstein2 opened this issue Oct 9, 2018 · 4 comments
Open

CallCredentials should respect the Deadline #4929

igorbernstein2 opened this issue Oct 9, 2018 · 4 comments
Milestone

Comments

@igorbernstein2
Copy link
Contributor

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
Copy link
Contributor

zhangkun83 commented Oct 10, 2018

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
Copy link
Contributor Author

igorbernstein2 commented Oct 10, 2018

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
Copy link
Contributor

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.

@zhangkun83 zhangkun83 added this to the 1.17 milestone Oct 12, 2018
@creamsoup creamsoup modified the milestones: 1.17, 1.18 Dec 4, 2018
@dapengzhang0 dapengzhang0 modified the milestones: 1.18, Next Dec 19, 2018
@dapengzhang0
Copy link
Member

Modified milestone to 'Next': Need assignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants