-
Notifications
You must be signed in to change notification settings - Fork 814
Refactor client factory channel and interceptor lifetime #1430
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
Refactor client factory channel and interceptor lifetime #1430
Conversation
| /// <param name="builder">The <see cref="IHttpClientBuilder"/>.</param> | ||
| /// <param name="configureInvoker">A delegate that is used to create an <see cref="Interceptor"/>.</param> | ||
| /// <returns>An <see cref="IHttpClientBuilder"/> that can be used to configure the client.</returns> | ||
| public static IHttpClientBuilder AddInterceptor(this IHttpClientBuilder builder, Func<Interceptor> configureInvoker) |
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.
Nit: document this has a Channel lifetime by default?
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.
Good idea.
I kind of want to rename InterceptorLifetime. It's more like a resolution scope. Someone could still register an interceptor as a singleton in DI and have a per-client interceptor be shared because it is a singleton in DI.
| InterceptorLifetime.Client); | ||
|
|
||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| if (clientFactoryOptions.Interceptors.Count != 0) |
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.
Is there a universe where we need to support a mix of interceptors registered on the obsolete Interceptors list and the new InterceptorRegistrations list?
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.
No. The only way the old Interceptors collection could have interceptors is if someone manually added them (i.e. didn't use the interceptor extension methods).
This is for backward compatibility until the old collection gets removed.
c7e5b22 to
8239428
Compare
|
@JamesNK when will a version with this merge wil be available? And is there a way to use the this approach to set the interceptor NOT as singleton? Here is the complete sample of the approach using CallCredentials.FromInterceptor: :#1430 br |
You can probably get it form the nightly feed now: https://github.com/grpc/grpc-dotnet#grpc-nuget-feed Your link to the sample didn't work. Can you re-link it? |
Sorry, here is the correct link: |
|
The sample below will always create a single instance because the interceptor is being configured as the channel credentials: services
.AddGrpcClient<Greeter.GreeterClient>(o =>
{
o.Address = new Uri("https://localhost:5001");
})
.ConfigureChannel(o =>
{
var credentials = CallCredentials.FromInterceptor((context, metadata) =>
{
if (!string.IsNullOrEmpty(_token))
{
metadata.Add("Authorization", $"Bearer {_token}");
}
return Task.CompletedTask;
});
o.Credentials = ChannelCredentials.Create(new SslCredentials(), credentials);
});What do you want the lifetime of an interceptor to be? |
I know that sample above creates a "singleton", but I want the interceptor to be resolved from the DI before every call, here is my issue I created a year ago, it's marked as fixed with your merge/pull request: |
|
A new interceptor will never be resolved before every call. But an interceptor that is registered with |
That would be fine as long as the channel stays the same/it's not created each time a new client is created. We have to provide/add a token for every call and this token changes from customer to customer. At the the moment we create a new client and a new channel for each request because we need the interceptor to be created as well because the interceptor gets a special info from the IOC so he can create the correct token and therefore this information must be received by the current IOC container/child container. The problem is that always creating a new channel is quite expensive/the first call to the grpc service is quite slow. When we create the channel once all subsequent grpc service calls are quite fast. But we can't use this approach because we can't calculate the correct token. Could you provide some more info how the |
|
Unit test example here: Interceptor is registered client factory using |
Thx for the Infos, just to be sure, the clients will be created per scope/request? And the interceptor will be created per scope/request? And the channel is only created once and shared between the clients? --> that would be very important for performance reason. Br |
|
Thx for your patience, I think that's exactly what I need. One last question: |
|
Created once per client type. If your app has GreeterClient and CounterClient then a maximum of two will be created. Although I might change that so a single channel is created for both if no channel settings are changed. I'll need to think about that. |
|
builder.Services |
Fixes #1264
Fixes #842
Fixes #1044
Client factory currently recreates the channel for every client instance that is created. This wasn't ideal, but all the heavy state for in a channel was in the HttpHandler, which was cached.
Features added in the last year mean channel now has state. Retries keep track of failed calls for throttling, and load balancing opens its own sockets. Recreating channel for each client instance is no longer an option.
This PR:
InterceptorLifetimesetting. By default, interceptors have a channel lifetime, so will be created once for a channel. But configuring an interceptor to have a client lifetime means it will get resolved per-client instance.