Skip to content
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

The ConfigureHttpClient method is not supported when creating gRPC clients. #4924

Closed
eerhardt opened this issue Feb 6, 2024 · 13 comments · Fixed by #5363 or dotnet/docs#42476
Closed
Assignees
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug.

Comments

@eerhardt
Copy link
Member

eerhardt commented Feb 6, 2024

Description

3654748 is breaking using gRPC clients with Http.Resilience. This is a breaking change from 8.1.

Reproduction Steps

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Http.Resilience" Version="8.2.0-preview.24105.1" />

    <PackageReference Include="Google.Protobuf" Version="3.25.2" />
    <PackageReference Include="Grpc.Net.ClientFactory" Version="2.59.0" />
    <PackageReference Include="Grpc.Tools" Version="2.59.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;

var builder = WebApplication.CreateBuilder(args);

// Add services to the container.

builder.Services.AddGrpcClient<Greeter.GreeterClient>(o =>
{
    o.Address = new Uri("https://localhost:5001");
});

builder.Services.ConfigureHttpClientDefaults(http =>
{
    // Turn on resilience by default
    http.AddStandardResilienceHandler();
});

var app = builder.Build();

// Configure the HTTP request pipeline.

var summaries = new[]
{
    "Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching"
};

app.MapGet("/hello", (Greeter.GreeterClient client) =>
{
    return "hello";
});

app.Run();

greet.proto:

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;
}

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:

An unhandled exception occurred while processing the request.
InvalidOperationException: The ConfigureHttpClient method is not supported when creating gRPC clients. Unable to create client with name 'GreeterClient'.
Grpc.Net.ClientFactory.Internal.GrpcCallInvokerFactory.CreateInvoker(EntryKey key)

Stack Query Cookies Headers Routing
InvalidOperationException: The ConfigureHttpClient method is not supported when creating gRPC clients. Unable to create client with name 'GreeterClient'.
Grpc.Net.ClientFactory.Internal.GrpcCallInvokerFactory.CreateInvoker(EntryKey key)
System.Collections.Concurrent.ConcurrentDictionary<TKey, TValue>.GetOrAdd(TKey key, Func<TKey, TValue> valueFactory)
Grpc.Net.ClientFactory.Internal.GrpcCallInvokerFactory.CreateInvoker(string name, Type type)
Grpc.Net.ClientFactory.Internal.DefaultGrpcClientFactory.CreateClient<TClient>(string name)
Microsoft.Extensions.DependencyInjection.GrpcClientServiceExtensions+<>c__DisplayClass7_0<TClient>.<AddGrpcHttpClient>b__2(IServiceProvider s)
Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite transientCallSite, RuntimeResolverContext context)
Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor<TArgument, TResult>.VisitCallSite(ServiceCallSite callSite, TArgument argument)
Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite callSite, ServiceProviderEngineScope scope)
Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine+<>c__DisplayClass2_0.<RealizeService>b__0(ServiceProviderEngineScope scope)
Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier serviceIdentifier, ServiceProviderEngineScope serviceProviderEngineScope)
Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)
Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
lambda_method2(Closure , object , HttpContext )
Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context)

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

@eerhardt eerhardt added untriaged bug This issue describes a behavior which is not expected - a bug. labels Feb 6, 2024
@joperezr
Copy link
Member

joperezr commented Feb 6, 2024

@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?

eerhardt added a commit to dotnet/aspire that referenced this issue Feb 7, 2024
* Revert "Update Microsoft.Extensions.Http.Resilience to 8.2 (#2094)"

This reverts commit 2bf302b.

There is a breaking change with Grpc clients in this version. See dotnet/extensions#4924

* Set the EndToEnd test HttpClient timeout to infinite

This allows the Http.Resilience handlers to handle the timeout correctly.
@martintmk
Copy link
Contributor

@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.

@eerhardt
Copy link
Member Author

eerhardt commented Feb 7, 2024

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?

radical pushed a commit to radical/aspire that referenced this issue Feb 7, 2024
* Revert "Update Microsoft.Extensions.Http.Resilience to 8.2 (dotnet#2094)"

This reverts commit 2bf302b.

There is a breaking change with Grpc clients in this version. See dotnet/extensions#4924

* Set the EndToEnd test HttpClient timeout to infinite

This allows the Http.Resilience handlers to handle the timeout correctly.
@JamesNK
Copy link
Member

JamesNK commented Feb 7, 2024

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.

@martintmk
Copy link
Contributor

@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?

@iliar-turdushev
Copy link
Contributor

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.

@JamesNK
Copy link
Member

JamesNK commented Apr 30, 2024

There aren't any updates. I've created an issue to track improving gRPC - grpc/grpc-dotnet#2425

I'm really busy so I'm not sure when I'll get time to look at it.

@iliar-turdushev
Copy link
Contributor

iliar-turdushev commented Aug 13, 2024

The issue in grpc-dotnet grpc/grpc-dotnet#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.

@RussKie
Copy link
Member

RussKie commented Aug 14, 2024

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.

iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Aug 14, 2024
Disables HttpClient Timeout for standard resilience and hending handlers
iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Aug 15, 2024
Adds a note mentioning requirements on the Grpc.Net.ClientFactory version to avoid issues with the M.E.Http.Resilience package
iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Aug 28, 2024
Added a target that notifies users that they use a version of the Grpc.Net.ClientFactory package that might cause the dotnet#4924 issue
iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Aug 30, 2024
Revert changes to the Directory.Build.targets
iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Sep 2, 2024
Adds a target that checks whether M.E.Http.Resilience package is used together with Grpc.Net.ClientFactory 2.64.0 or later. If not the target warns a user.
iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Sep 2, 2024
Adds a Known issues section to the doc describing the issue with Grpc.Net.ClientFactory
iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Sep 3, 2024
* Moves the contents of the .props file into the .targets file
* For net462 we now import the contents of the .targets file instead of setting it as a CDATA value in the .csproj file
iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Sep 3, 2024
Replaces the name of the project file with the MSBuildProjectName variable
iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Sep 4, 2024
* Add conditions to pack buildTransitive .targets for net462 only when net462 is included as a target framework
* Changed the documentation link to the learn.microsoft.com site
iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Sep 4, 2024
* Applies editorial changes to the Known issues section
iliar-turdushev added a commit to iliar-turdushev/dotnet-extensions that referenced this issue Sep 5, 2024
* Changes the level of the compatibility log messages from Error to Warning
* Updates the logic of copying buildTransitive files
@iliar-turdushev
Copy link
Contributor

To reproduce the issue on NET Framework 4.6.2 we can use the following steps:

  1. Apply to dotnet/extensions locally the following changes from the commit 3654748.

  2. Build the solution and publish the packages locally: .\build.cmd -pack.

  3. 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>
  1. Add Program.cs file:
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>();
        }
    }
}
  1. Add greet.proto file:
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;
}
  1. Run the application, you'll get the following exception:
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)

@JamesNK
Copy link
Member

JamesNK commented Sep 6, 2024

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.

@iliar-turdushev
Copy link
Contributor

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.

@JamesNK
Copy link
Member

JamesNK commented Sep 6, 2024

Ok. Just saying, I think you can close this issue. The problem was in grpc and it's been fixed there and released.

IEvangelist added a commit to dotnet/docs that referenced this issue Sep 6, 2024
….Net.ClientFactory library (#42476)

* Adds a "Known issues" section describing potential issue described here dotnet/extensions#4924

* Apply suggestions from code review

---------

Co-authored-by: David Pine <[email protected]>
iliar-turdushev added a commit that referenced this issue Sep 9, 2024
* Fixes #4924 and #4770

Disables HttpClient Timeout for standard resilience and hending handlers

* Fixes #4924

Adds a note mentioning requirements on the Grpc.Net.ClientFactory version to avoid issues with the M.E.Http.Resilience package

* Fixes #4924

Added a target that notifies users that they use a version of the Grpc.Net.ClientFactory package that might cause the #4924 issue

* Fixes #4924

Revert changes to the Directory.Build.targets

* Fixes #4924

Adds a target that checks whether M.E.Http.Resilience package is used together with Grpc.Net.ClientFactory 2.64.0 or later. If not the target warns a user.

* Fixes #4924

Adds a Known issues section to the doc describing the issue with Grpc.Net.ClientFactory

* Fixes #4924

* Moves the contents of the .props file into the .targets file
* For net462 we now import the contents of the .targets file instead of setting it as a CDATA value in the .csproj file

* Fixes #4924

Replaces the name of the project file with the MSBuildProjectName variable

* Fixes #4924

* Add conditions to pack buildTransitive .targets for net462 only when net462 is included as a target framework
* Changed the documentation link to the learn.microsoft.com site

* Fixes #4924

* Applies editorial changes to the Known issues section

* Fixes #4924

* Changes the level of the compatibility log messages from Error to Warning
* Updates the logic of copying buildTransitive files

* Removed extra spaces

* Applied suggestions to update the documentation

* Removed locale from the URL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
8 participants