Skip to content

Set context in executor when calling Credentials.getRequestMetadata #5334

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
pkwarren opened this issue Feb 7, 2019 · 4 comments
Open
Assignees
Milestone

Comments

@pkwarren
Copy link
Contributor

pkwarren commented Feb 7, 2019

What version of gRPC are you using?

1.18.0

What did you expect to see?

I've written a custom implementation of a com.google.auth.Credentials class which retrieves tokens from etcd (using their v3 gRPC api). I'm attempting to trace these method calls by using the Context to link work done on separate threads. I've discovered that the Context is not available in my Credentials class (specifically when getRequestMetadata is called using an executor).

I've been able to work around this temporarily by changing:

kvStub.withDeadlineAfter(5, TimeUnit.SECONDS)
    .withCallCredentials(creds)
    .range(rangeRequest, object : StreamObserver<RangeResponse> {

to:

kvStub.withDeadlineAfter(5, TimeUnit.SECONDS)
    .withCallCredentials(creds)
    .withExecutor(Context.currentContextExecutor(Executors.newCachedThreadPool()))
    .range(rangeRequest, object : StreamObserver<RangeResponse> {

Is it expected that the Context be available for use in a custom Credentials implementation? By doing so, this might also help to close some other open issues (#4929) and also allow deadline propagation to work properly when authenticating against a gRPC api.

@carl-mastrangelo
Copy link
Contributor

Seems simple enough to fix in CallCredentialsApplyingTransportFactory, not sure about implications. @ejona86 SGTY?

@ejona86
Copy link
Member

ejona86 commented Feb 7, 2019

CC @zhangkun83

There may be two possible reasons for this problem:

  1. The context is not set when calling newStream. It is being done from ClientCallImpl and DelayedClientTransport, so this doesn't seem to be the cause
  1. The context is not copied to the executor, for blocking operations. This is certainly the case, although you could argue that the Credentials.getRequestMetadata class (or a child class) should propagate the context when calling blockingGetToCallback from the executor.

I don't expect we'll get Credentials itself to propagate the Context, which leaves three options:

  1. CallCredentialsApplyingTransportFactory wraps the executor
  2. GoogleAuthLibraryCallCredentials wraps the executor
  3. @pkwarren's Credentials propagates the Context within getRequestMetadata()

For 1 and 2, there's also the question of whether it should be a Context.fixedContextExecutor() or Context.currentContextExecutor().

Interestingly, would are almost using fixedContextExecutor() today, as ClientCallImpl uses ContextRunnables instead of fixedContextExecutor() for reduced allocations. However, CallCredentialsApplyingTransportFactory re-discovers the executor; if ClientCallImpl wrapped the executor it wouldn't notice.

In general, using Context.currentContextExecutor() should be preferred over Context.fixedContextExecutor().

@ejona86
Copy link
Member

ejona86 commented Feb 7, 2019

I think I'm leaning toward CallCredentialsApplyingTransportFactory wrapping the executor with Context.currentContextExecutor().

@zhangkun83
Copy link
Contributor

SGTM

@zhangkun83 zhangkun83 self-assigned this Feb 7, 2019
@dapengzhang0 dapengzhang0 added this to the Next milestone Feb 9, 2019
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

5 participants