From 8546264b7e9f8ef6b5836c26c1fd38c8b54c8079 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Fri, 28 Apr 2023 16:55:37 +0800 Subject: [PATCH 1/5] Make CancellationToken available in call credentials interceptor --- .../Internal/HttpContextServerCallContext.cs | 17 +----- src/Grpc.Core.Api/AsyncAuthInterceptor.cs | 24 +++++++- .../DefaultCallCredentialsConfigurator.cs | 13 +++-- src/Grpc.Net.Client/Internal/GrpcCall.cs | 4 +- .../Internal/GrpcProtocolHelpers.cs | 56 +++++++++++++------ .../CallCredentialTests.cs | 55 +++++++++++++++++- 6 files changed, 125 insertions(+), 44 deletions(-) diff --git a/src/Grpc.AspNetCore.Server/Internal/HttpContextServerCallContext.cs b/src/Grpc.AspNetCore.Server/Internal/HttpContextServerCallContext.cs index d525e99ac..1e0cde734 100644 --- a/src/Grpc.AspNetCore.Server/Internal/HttpContextServerCallContext.cs +++ b/src/Grpc.AspNetCore.Server/Internal/HttpContextServerCallContext.cs @@ -77,12 +77,7 @@ protected override string PeerCore get { // Follows the standard at https://github.com/grpc/grpc/blob/master/doc/naming.md - if (_peer == null) - { - _peer = BuildPeer(); - } - - return _peer; + return _peer ??= BuildPeer(); } } @@ -291,10 +286,7 @@ private void EndCallCore() private void LogCallEnd() { - if (_activity != null) - { - _activity.AddTag(GrpcServerConstants.ActivityStatusCodeTag, _status.StatusCode.ToTrailerString()); - } + _activity?.AddTag(GrpcServerConstants.ActivityStatusCodeTag, _status.StatusCode.ToTrailerString()); if (_status.StatusCode != StatusCode.OK) { if (GrpcEventSource.Log.IsEnabled()) @@ -387,10 +379,7 @@ protected override Task WriteResponseHeadersAsyncCore(Metadata responseHeaders) public void Initialize(ISystemClock? clock = null) { _activity = GetHostActivity(); - if (_activity != null) - { - _activity.AddTag(GrpcServerConstants.ActivityMethodTag, MethodCore); - } + _activity?.AddTag(GrpcServerConstants.ActivityMethodTag, MethodCore); if (GrpcEventSource.Log.IsEnabled()) { diff --git a/src/Grpc.Core.Api/AsyncAuthInterceptor.cs b/src/Grpc.Core.Api/AsyncAuthInterceptor.cs index e7ca60c72..b63312cd8 100644 --- a/src/Grpc.Core.Api/AsyncAuthInterceptor.cs +++ b/src/Grpc.Core.Api/AsyncAuthInterceptor.cs @@ -16,6 +16,7 @@ #endregion +using System.Threading; using System.Threading.Tasks; using Grpc.Core.Utils; @@ -34,16 +35,25 @@ namespace Grpc.Core; /// public class AuthInterceptorContext { - readonly string serviceUrl; - readonly string methodName; + private readonly string serviceUrl; + private readonly string methodName; + private readonly CancellationToken cancellationToken; /// /// Initializes a new instance of AuthInterceptorContext. /// - public AuthInterceptorContext(string serviceUrl, string methodName) + public AuthInterceptorContext(string serviceUrl, string methodName) : this(serviceUrl, methodName, CancellationToken.None) + { + } + + /// + /// Initializes a new instance of AuthInterceptorContext. + /// + public AuthInterceptorContext(string serviceUrl, string methodName, CancellationToken cancellationToken) { this.serviceUrl = GrpcPreconditions.CheckNotNull(serviceUrl, nameof(serviceUrl)); this.methodName = GrpcPreconditions.CheckNotNull(methodName, nameof(methodName)); + this.cancellationToken = cancellationToken; } /// @@ -61,4 +71,12 @@ public string MethodName { get { return methodName; } } + + /// + /// The cancellation token of the RPC being called. + /// + public CancellationToken CancellationToken + { + get { return cancellationToken; } + } } diff --git a/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs b/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs index fbed1d556..4fc3064d4 100644 --- a/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs +++ b/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs @@ -1,4 +1,4 @@ -#region Copyright notice and license +#region Copyright notice and license // Copyright 2019 The gRPC Authors // @@ -20,15 +20,18 @@ namespace Grpc.Net.Client.Internal; -internal class DefaultCallCredentialsConfigurator : CallCredentialsConfiguratorBase +internal sealed class DefaultCallCredentialsConfigurator : CallCredentialsConfiguratorBase { public AsyncAuthInterceptor? Interceptor { get; private set; } - public IReadOnlyList? Credentials { get; private set; } + public IReadOnlyList? CompositeCredentials { get; private set; } + + // A place to cache context to avoid creating a new context for each auth interceptor call. + public AuthInterceptorContext? CachedContext { get; set; } public void Reset() { Interceptor = null; - Credentials = null; + CompositeCredentials = null; } public override void SetAsyncAuthInterceptorCredentials(object? state, AsyncAuthInterceptor interceptor) @@ -38,6 +41,6 @@ public override void SetAsyncAuthInterceptorCredentials(object? state, AsyncAuth public override void SetCompositeCredentials(object? state, IReadOnlyList credentials) { - Credentials = credentials; + CompositeCredentials = credentials; } } diff --git a/src/Grpc.Net.Client/Internal/GrpcCall.cs b/src/Grpc.Net.Client/Internal/GrpcCall.cs index d6a8170cf..693f79cfe 100644 --- a/src/Grpc.Net.Client/Internal/GrpcCall.cs +++ b/src/Grpc.Net.Client/Internal/GrpcCall.cs @@ -955,13 +955,13 @@ private async Task ReadCredentials(HttpRequestMessage request) if (Options.Credentials != null) { - await GrpcProtocolHelpers.ReadCredentialMetadata(configurator, Channel, request, Method, Options.Credentials).ConfigureAwait(false); + await GrpcProtocolHelpers.ReadCredentialMetadata(configurator, Channel, request, Method, Options.Credentials, _callCts.Token).ConfigureAwait(false); } if (Channel.CallCredentials?.Count > 0) { foreach (var credentials in Channel.CallCredentials) { - await GrpcProtocolHelpers.ReadCredentialMetadata(configurator, Channel, request, Method, credentials).ConfigureAwait(false); + await GrpcProtocolHelpers.ReadCredentialMetadata(configurator, Channel, request, Method, credentials, _callCts.Token).ConfigureAwait(false); } } } diff --git a/src/Grpc.Net.Client/Internal/GrpcProtocolHelpers.cs b/src/Grpc.Net.Client/Internal/GrpcProtocolHelpers.cs index 2794d2c1f..e2d7101f4 100644 --- a/src/Grpc.Net.Client/Internal/GrpcProtocolHelpers.cs +++ b/src/Grpc.Net.Client/Internal/GrpcProtocolHelpers.cs @@ -1,4 +1,4 @@ -#region Copyright notice and license +#region Copyright notice and license // Copyright 2019 The gRPC Authors // @@ -121,13 +121,34 @@ internal static bool ShouldSkipHeader(string name) /* round an integer up to the next value with three significant figures */ private static long TimeoutRoundUpToThreeSignificantFigures(long x) { - if (x < 1000) return x; - if (x < 10000) return RoundUp(x, 10); - if (x < 100000) return RoundUp(x, 100); - if (x < 1000000) return RoundUp(x, 1000); - if (x < 10000000) return RoundUp(x, 10000); - if (x < 100000000) return RoundUp(x, 100000); - if (x < 1000000000) return RoundUp(x, 1000000); + if (x < 1000) + { + return x; + } + if (x < 10000) + { + return RoundUp(x, 10); + } + if (x < 100000) + { + return RoundUp(x, 100); + } + if (x < 1000000) + { + return RoundUp(x, 1000); + } + if (x < 10000000) + { + return RoundUp(x, 10000); + } + if (x < 100000000) + { + return RoundUp(x, 100000); + } + if (x < 1000000000) + { + return RoundUp(x, 1000000); + } return RoundUp(x, 10000000); static long RoundUp(long x, long divisor) @@ -235,7 +256,7 @@ internal static bool CanWriteCompressed(WriteOptions? writeOptions) return canCompress; } - internal static AuthInterceptorContext CreateAuthInterceptorContext(Uri baseAddress, IMethod method) + internal static AuthInterceptorContext CreateAuthInterceptorContext(Uri baseAddress, IMethod method, CancellationToken cancellationToken) { var authority = baseAddress.Authority; if (baseAddress.Scheme == Uri.UriSchemeHttps && authority.EndsWith(":443", StringComparison.Ordinal)) @@ -252,7 +273,7 @@ internal static AuthInterceptorContext CreateAuthInterceptorContext(Uri baseAddr serviceUrl += "/"; } serviceUrl += method.ServiceName; - return new AuthInterceptorContext(serviceUrl, method.Name); + return new AuthInterceptorContext(serviceUrl, method.Name, cancellationToken); } internal static async Task ReadCredentialMetadata( @@ -260,15 +281,16 @@ internal static async Task ReadCredentialMetadata( GrpcChannel channel, HttpRequestMessage message, IMethod method, - CallCredentials credentials) + CallCredentials credentials, + CancellationToken cancellationToken) { credentials.InternalPopulateConfiguration(configurator, null); if (configurator.Interceptor != null) { - var authInterceptorContext = GrpcProtocolHelpers.CreateAuthInterceptorContext(channel.Address, method); + configurator.CachedContext ??= CreateAuthInterceptorContext(channel.Address, method, cancellationToken); var metadata = new Metadata(); - await configurator.Interceptor(authInterceptorContext, metadata).ConfigureAwait(false); + await configurator.Interceptor(configurator.CachedContext, metadata).ConfigureAwait(false); foreach (var entry in metadata) { @@ -276,14 +298,14 @@ internal static async Task ReadCredentialMetadata( } } - if (configurator.Credentials != null) + if (configurator.CompositeCredentials != null) { // Copy credentials locally. ReadCredentialMetadata will update it. - var callCredentials = configurator.Credentials; - foreach (var c in callCredentials) + var compositeCredentials = configurator.CompositeCredentials; + foreach (var callCredentials in compositeCredentials) { configurator.Reset(); - await ReadCredentialMetadata(configurator, channel, message, method, c).ConfigureAwait(false); + await ReadCredentialMetadata(configurator, channel, message, method, callCredentials, cancellationToken).ConfigureAwait(false); } } } diff --git a/test/Grpc.Net.Client.Tests/CallCredentialTests.cs b/test/Grpc.Net.Client.Tests/CallCredentialTests.cs index b80d6d6e6..4bbe0bf65 100644 --- a/test/Grpc.Net.Client.Tests/CallCredentialTests.cs +++ b/test/Grpc.Net.Client.Tests/CallCredentialTests.cs @@ -1,4 +1,4 @@ -#region Copyright notice and license +#region Copyright notice and license // Copyright 2019 The gRPC Authors // @@ -18,6 +18,7 @@ using System.Net; using System.Net.Http.Headers; +using System.Threading; using Greet; using Grpc.Core; using Grpc.Net.Client.Tests.Infrastructure; @@ -79,19 +80,67 @@ public async Task CallCredentialsWithHttps_MetadataOnRequest() var invoker = HttpClientCallInvokerFactory.Create(httpClient); // Act + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var callCredentials = CallCredentials.FromInterceptor(async (context, metadata) => { // The operation is asynchronous to ensure delegate is awaited - await Task.Delay(50); + + // Ensure task hasn't been completed. + Assert.False(tcs.Task.IsCompleted); + // Wait for TCS to be completed. + await tcs.Task; + + // Set header. metadata.Add("authorization", "SECRET_TOKEN"); }); var call = invoker.AsyncUnaryCall(ClientTestHelpers.ServiceMethod, string.Empty, new CallOptions(credentials: callCredentials), new HelloRequest()); - await call.ResponseAsync.DefaultTimeout(); + var responseTask = call.ResponseAsync; + + tcs.SetResult(null); + await responseTask.DefaultTimeout(); // Assert Assert.AreEqual("SECRET_TOKEN", authorizationValue); } + [Test] + public async Task CallCredentialsWithHttps_CancellationToken() + { + // Arrange + string? authorizationValue = null; + var httpClient = ClientTestHelpers.CreateTestClient(async request => + { + authorizationValue = request.Headers.GetValues("authorization").Single(); + + var reply = new HelloReply { Message = "Hello world" }; + var streamContent = await ClientTestHelpers.CreateResponseContent(reply).DefaultTimeout(); + return ResponseUtils.CreateResponse(HttpStatusCode.OK, streamContent); + }); + var invoker = HttpClientCallInvokerFactory.Create(httpClient); + + // Act + var unreachableAuthInterceptorSection = false; + var callCredentials = CallCredentials.FromInterceptor(async (context, metadata) => + { + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + context.CancellationToken.Register(s => ((TaskCompletionSource)s!).SetCanceled(), tcs); + + await tcs.Task; + + unreachableAuthInterceptorSection = true; + }); + var call = invoker.AsyncUnaryCall(ClientTestHelpers.ServiceMethod, string.Empty, new CallOptions(credentials: callCredentials), new HelloRequest()); + var responseTask = call.ResponseAsync; + + call.Dispose(); + + var ex = await ExceptionAssert.ThrowsAsync(() => responseTask).DefaultTimeout(); + Assert.AreEqual(StatusCode.Cancelled, ex.StatusCode); + + // Assert + Assert.False(unreachableAuthInterceptorSection); + } + [Test] public async Task CallCredentialsWithHttp_NoMetadataOnRequest() { From ab79e50094289881c2eec6ef618b0e95ef22cca1 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sat, 29 Apr 2023 10:35:56 +0800 Subject: [PATCH 2/5] Update --- .../Internal/DefaultCallCredentialsConfigurator.cs | 4 ++-- src/Grpc.Net.Client/Internal/GrpcProtocolHelpers.cs | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs b/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs index 4fc3064d4..8e9d8ddc0 100644 --- a/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs +++ b/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs @@ -25,10 +25,10 @@ internal sealed class DefaultCallCredentialsConfigurator : CallCredentialsConfig public AsyncAuthInterceptor? Interceptor { get; private set; } public IReadOnlyList? CompositeCredentials { get; private set; } - // A place to cache context to avoid creating a new context for each auth interceptor call. + // A place to cache the context to avoid creating a new instance for each auth interceptor call. public AuthInterceptorContext? CachedContext { get; set; } - public void Reset() + public void ResetPerCallCredentialState() { Interceptor = null; CompositeCredentials = null; diff --git a/src/Grpc.Net.Client/Internal/GrpcProtocolHelpers.cs b/src/Grpc.Net.Client/Internal/GrpcProtocolHelpers.cs index e2d7101f4..7935cedc1 100644 --- a/src/Grpc.Net.Client/Internal/GrpcProtocolHelpers.cs +++ b/src/Grpc.Net.Client/Internal/GrpcProtocolHelpers.cs @@ -288,7 +288,12 @@ internal static async Task ReadCredentialMetadata( if (configurator.Interceptor != null) { + // Multiple auth interceptors can be called for a gRPC call. + // These all have the same data: address, method and cancellation token. + // Lazily allocate the context if it is needed. + // Stored on the configurator instead of a ref parameter because ref parameters are not supported in async methods. configurator.CachedContext ??= CreateAuthInterceptorContext(channel.Address, method, cancellationToken); + var metadata = new Metadata(); await configurator.Interceptor(configurator.CachedContext, metadata).ConfigureAwait(false); @@ -304,7 +309,7 @@ internal static async Task ReadCredentialMetadata( var compositeCredentials = configurator.CompositeCredentials; foreach (var callCredentials in compositeCredentials) { - configurator.Reset(); + configurator.ResetPerCallCredentialState(); await ReadCredentialMetadata(configurator, channel, message, method, callCredentials, cancellationToken).ConfigureAwait(false); } } From afba0b1d60904232d310641594cfaca1e5aa787c Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sat, 29 Apr 2023 10:37:45 +0800 Subject: [PATCH 3/5] Update --- .../Internal/DefaultCallCredentialsConfigurator.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs b/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs index 8e9d8ddc0..c526be758 100644 --- a/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs +++ b/src/Grpc.Net.Client/Internal/DefaultCallCredentialsConfigurator.cs @@ -26,6 +26,7 @@ internal sealed class DefaultCallCredentialsConfigurator : CallCredentialsConfig public IReadOnlyList? CompositeCredentials { get; private set; } // A place to cache the context to avoid creating a new instance for each auth interceptor call. + // It's ok not to reset this state because the context is only used for the lifetime of the call. public AuthInterceptorContext? CachedContext { get; set; } public void ResetPerCallCredentialState() From 98190f0e1a2733e3d94235c583586726dc49abae Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sat, 29 Apr 2023 10:54:50 +0800 Subject: [PATCH 4/5] PR feedback --- .../CallCredentialTests.cs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/test/Grpc.Net.Client.Tests/CallCredentialTests.cs b/test/Grpc.Net.Client.Tests/CallCredentialTests.cs index 4bbe0bf65..ea5c8a98e 100644 --- a/test/Grpc.Net.Client.Tests/CallCredentialTests.cs +++ b/test/Grpc.Net.Client.Tests/CallCredentialTests.cs @@ -80,15 +80,12 @@ public async Task CallCredentialsWithHttps_MetadataOnRequest() var invoker = HttpClientCallInvokerFactory.Create(httpClient); // Act - var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var syncPoint = new SyncPoint(runContinuationsAsynchronously: true); var callCredentials = CallCredentials.FromInterceptor(async (context, metadata) => { - // The operation is asynchronous to ensure delegate is awaited + // The operation is asynchronous to ensure delegate is awaited. - // Ensure task hasn't been completed. - Assert.False(tcs.Task.IsCompleted); - // Wait for TCS to be completed. - await tcs.Task; + await syncPoint.WaitToContinue(); // Set header. metadata.Add("authorization", "SECRET_TOKEN"); @@ -96,7 +93,14 @@ public async Task CallCredentialsWithHttps_MetadataOnRequest() var call = invoker.AsyncUnaryCall(ClientTestHelpers.ServiceMethod, string.Empty, new CallOptions(credentials: callCredentials), new HelloRequest()); var responseTask = call.ResponseAsync; - tcs.SetResult(null); + await syncPoint.WaitForSyncPoint().DefaultTimeout(); + + // Response task should be blocked waiting for the auth interceptor to complete. + Assert.False(responseTask.IsCompleted); + // Sending the request should be blocked waiting for the auth interceptor to complete. + Assert.Null(authorizationValue); + + syncPoint.Continue(); await responseTask.DefaultTimeout(); // Assert From 8ac2283c649b28ba0ebc5799f96e1cc9539aa8a4 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sat, 29 Apr 2023 10:55:43 +0800 Subject: [PATCH 5/5] Comment --- test/Grpc.Net.Client.Tests/CallCredentialTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Grpc.Net.Client.Tests/CallCredentialTests.cs b/test/Grpc.Net.Client.Tests/CallCredentialTests.cs index ea5c8a98e..eb2678c6d 100644 --- a/test/Grpc.Net.Client.Tests/CallCredentialTests.cs +++ b/test/Grpc.Net.Client.Tests/CallCredentialTests.cs @@ -83,8 +83,8 @@ public async Task CallCredentialsWithHttps_MetadataOnRequest() var syncPoint = new SyncPoint(runContinuationsAsynchronously: true); var callCredentials = CallCredentials.FromInterceptor(async (context, metadata) => { - // The operation is asynchronous to ensure delegate is awaited. - + // The operation is asynchronous to ensure auth interceptor is awaited. + // Sending the request and returning a response is blocked until the auth interceptor completes. await syncPoint.WaitToContinue(); // Set header.