-
Notifications
You must be signed in to change notification settings - Fork 781
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
Make CancellationToken available in call credentials interceptor #2107
Make CancellationToken available in call credentials interceptor #2107
Conversation
{ | ||
credentials.InternalPopulateConfiguration(configurator, null); | ||
|
||
if (configurator.Interceptor != null) | ||
{ | ||
var authInterceptorContext = GrpcProtocolHelpers.CreateAuthInterceptorContext(channel.Address, method); | ||
configurator.CachedContext ??= CreateAuthInterceptorContext(channel.Address, method, cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looked scary at first, but the call sites don't reuse the DefaultCallCredentialsConfigurator
for different channel + method + cancellation combos.
Can we make that more obvious somehow so this code doesn't look scary?
e.g.
ReadCredentialMetadata(...)
{
ReadCredentialMetadataCore(...);
configurator.CachedContext = null;
}
// Rename current method
ReadCredentialMetadataInner(...)
{
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated comments and renamed some fields to make it clearer what is going on.
await Task.Delay(50); | ||
|
||
// Ensure task hasn't been completed. | ||
Assert.False(tcs.Task.IsCompleted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit odd. I think in theory the compiler or synccontext could choose to run this asynchronously before it gets here in which case it would be a race.
Generally you would instead use 2 tcs's to ensure order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test was fine. The tcs couldn't be completed until after the auth interceptor awaits. The assert was to verify that behavior.
But I agree the test looks a little weird. And it's brittle. I've switched it to use a sync point.
Addresses #2108
Adds
AuthInterceptorContext.CancellationToken
property. The context is passed to call credential interceptors.This change allows call credentials that do async work to be canceled when the gRPC call is canceled: