Closed eerhardt closed 2 months ago
@martintmk can you please take a look at this? As specified above, this is a regression and breaking change, and we are preparing to lock for 8.2 release, so time is running out for fixing this. If fixing this is not trivial, I'd suggest reverting the change that introduced the regression for 8.2, and then we can check on how to better fix the original issue without causing a regression for 8.3. Thoughts?
@joperezr Let's just revert that fix for 8.2.0, service owners can still disable the timeout as a workaround.
Unless the restrictions around using ConfigureHttpClient
is lifted for GRPC clients, I don't see any other way to fix this. (other than manually disabling timeout for HttpClient
.
I must say the behavior of ConfigureHttpClient
is strange, would love know the reason why it is not allowed.
I must say the behavior of ConfigureHttpClient is strange, would love know the reason why it is not allowed.
+1.
@JamesNK, can you clue us in on why this is restricted?
I must say the behavior of ConfigureHttpClient is strange, would love know the reason why it is not allowed.
+1.
@JamesNK, can you clue us in on why this is restricted?
Because it has no impact. It's to let the developer know that this configuration they added to the gRPC client won't do anything.
builder.Services.AddGrpcClient<Greeter.GreeterClient>(o =>
{
o.Address = new Uri("https://localhost:5001/");
})
.ConfigureHttpClient(options => options.Timeout = TimeSpan.FromSeconds(10)); // no impact
However, now that HttpClientFactory has the concept of applying defaults across all clients it isn't as useful. Changes would be to either add an option to gRPC client factory to suppress the error or changing the error to logging.
@JamesNK
Changes would be to either add an option to gRPC client factory to suppress the error or changing the error to logging.
My vote would be to apply this change and once the new version of the library (gRPC ??) is in we can revert #4925 again and re-introduce the fix.
@eerhardt Thoughts?
However, now that HttpClientFactory has the concept of applying defaults across all clients it isn't as useful. Changes would be to either add an option to gRPC client factory to suppress the error or changing the error to logging.
@JamesNK Are there any updates to the suggestion above (suppress the error or log it)? If not, is there a ticket/item to tract it? Thank you.
There aren't any updates. I've created an issue to track improving gRPC - https://github.com/grpc/grpc-dotnet/issues/2425
I'm really busy so I'm not sure when I'll get time to look at it.
The issue in grpc-dotnet https://github.com/grpc/grpc-dotnet/issues/2425 was fixed in v2.64.0. We can now reintroduce #4770, and it will not cause the current issue when used together with grpc-dotnet >=v2.64.0.
@joperezr @RussKie How do you think if we need to mention in our documentation (or release notes) that our clients need to be on grpc-dotnet >=v2.64.0 not to face the current issue? Or is it enough to mention it here in the current ticket? I personally think that it should be in release notes, and we don't have to include it in our documentation. Thank you.
dotnet tools, especially those installed globally, aren't updated often (by the user). So, the users likely won't know to update those. It's ok to have the note here and close the issue, if it's resolved by updating to the latest version of grpc-dotnet. However, this isn't very discoverable, and we may see similar issues raised in the future.
To advertise the fix more broadly, I think, we could add a note to https://github.com/dotnet/extensions/blob/main/src/Libraries/Microsoft.Extensions.Http.Resilience/README.md (before the "Install the package" section). We should also add the same note to https://learn.microsoft.com/dotnet/core/resilience/http-resilience (in the "Get started" section perhaps?).
This won't guarantee that users will necessarily read the docs, but at least we'll be able to close similar issues pointing the users to the docs. We can't really expect users to trawl through our GitHub history looking for a fix.
To reproduce the issue on NET Framework 4.6.2 we can use the following steps:
Apply to dotnet/extensions locally the following changes from the commit https://github.com/dotnet/extensions/commit/3654748e14445fa794bcdb0d2ebffa46e758603c.
Build the solution and publish the packages locally: .\build.cmd -pack
.
Create a new project in a separate solution. Please, note that we are referencing local package Microsoft.Extensions.Http.Resilience 8.10.0-dev. You need to set up NuGet source to use local folder with dotnet/extensions packages and, probably, adjust the version of the package to your local version.
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net462</TargetFramework>
<SuppressCheckGrpcNetClientFactoryVersion>false</SuppressCheckGrpcNetClientFactoryVersion>
</PropertyGroup>
<ItemGroup>
<None Remove="Protos\greet.proto" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Http.Resilience" Version="8.10.0-dev" />
<PackageReference Include="Grpc.Net.ClientFactory" Version="2.62.0" />
<PackageReference Include="Google.Protobuf" Version="3.27.0" />
<PackageReference Include="Grpc.Tools" Version="2.62.0">
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
<PrivateAssets>all</PrivateAssets>
</PackageReference>
</ItemGroup>
<ItemGroup>
<Protobuf Include="Protos\greet.proto" GrpcServices="Client" />
</ItemGroup>
</Project>
using GrpcGreeter;
using Microsoft.Extensions.DependencyInjection;
using System;
using System.Threading;
namespace ConsoleApp
{
internal static class Program
{
private static void Main(string[] args)
{
var serviceCollection = new ServiceCollection();
serviceCollection.ConfigureHttpClientDefaults(http =>
{
http.AddStandardResilienceHandler();
});
serviceCollection.AddGrpcClient<Greeter.GreeterClient>(o =>
{
o.Address = new Uri("https://localhost:5001");
});
var serviceProvider = serviceCollection.BuildServiceProvider();
var grpcClient = serviceProvider.GetRequiredService<Greeter.GreeterClient>();
}
}
}
syntax = "proto3";
option csharp_namespace = "GrpcGreeter";
package greet;
// The greeting service definition.
service Greeter {
// Sends a greeting
rpc SayHello (HelloRequest) returns (HelloReply);
}
// The request message containing the user's name.
message HelloRequest {
string name = 1;
}
// The response message containing the greetings.
message HelloReply {
string message = 1;
}
Unhandled Exception: System.InvalidOperationException: The ConfigureHttpClient method is not supported when creating gRPC clients. Unable to create client with name 'GreeterClient'.
at Grpc.Net.ClientFactory.Internal.GrpcCallInvokerFactory.CreateInvoker(EntryKey key)
at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
at Grpc.Net.ClientFactory.Internal.DefaultGrpcClientFactory.CreateClient[TClient](String name)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitFactory(FactoryCallSite factoryCallSite, RuntimeResolverContext context)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite transientCallSite, RuntimeResolverContext context)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite callSite, ServiceProviderEngineScope scope)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine.<>c__DisplayClass2_0.<RealizeService>b__0(ServiceProviderEngineScope scope)
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier serviceIdentifier, ServiceProviderEngineScope serviceProviderEngineScope)
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(Type serviceType)
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
at ConsoleApp.Program.Main(String[] args)
I think this works if you use gRPC 2.64.0 or later. It would be great if you could update your grpc package versions to the latest and see whether this is still a problem.
I think this works if you use gRPC 2.64.0 or later. It would be great if you could update your grpc package versions to the latest and see whether this is still a problem.
Correct, we just wanted to have here steps how to reproduce the issue on net462.
Ok. Just saying, I think you can close this issue. The problem was in grpc and it's been fixed there and released.
Description
https://github.com/dotnet/extensions/commit/3654748e14445fa794bcdb0d2ebffa46e758603c is breaking using gRPC clients with Http.Resilience. This is a breaking change from 8.1.
Reproduction Steps
greet.proto:
Expected behavior
The app should work, like it did when I use
<PackageReference Include="Microsoft.Extensions.Http.Resilience" Version="8.2.0-preview.24105.1" />
Actual behavior
The app fails with:
Regression?
Yes, from 8.1
Known Workarounds
Don't call
AddStandardResilienceHandler()
on HttpClients that use gRPC.Configuration
net8.0
Other information
cc @martintmk @geeknoid @joperezr @JamesNK