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

Revert http resilience upgrade #2107

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Feb 6, 2024

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

InvalidOperationException: The ConfigureHttpClient method is not supported when creating gRPC clients. Unable to create client with name 'BasketClient'.
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.CallSiteVisitor<TArgument, TResult>.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
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.VisitConstructor(ConstructorCallSite constructorCallSite, RuntimeResolverContext context)
Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor<TArgument, TResult>.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitRootCache(ServiceCallSite callSite, 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.ServiceProvider.CreateServiceAccessor(ServiceIdentifier serviceIdentifier)
System.Collections.Concurrent.ConcurrentDictionary<TKey, TValue>.GetOrAdd(TKey key, Func<TKey, TValue> valueFactory)
Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier serviceIdentifier, ServiceProviderEngineScope serviceProviderEngineScope)
Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)
Microsoft Reviewers: Open in CodeFlow

…)"

This reverts commit 2bf302b.

There is a breaking change with Grpc clients in this version.
This allows the Http.Resilience handlers to handle the timeout correctly.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Feb 6, 2024
@radical
Copy link
Member

radical commented Feb 6, 2024

Would it be possible to also add an explicit unit test for this, so it doesn't we don't depend on running the actual app to catch it?

@eerhardt
Copy link
Member Author

eerhardt commented Feb 6, 2024

Would it be possible to also add an explicit unit test for this, so it doesn't we don't depend on running the actual app to catch it?

What exactly do you imagine us testing? This is a break between Http.Resilience (in dotnet/extensions) and gRPC client (which we don't own). .NET Aspire doesn't do anything with gRPC by default, we just happen to use it in one of our "playground apps".

@radical
Copy link
Member

radical commented Feb 6, 2024

Would it be possible to also add an explicit unit test for this, so it doesn't we don't depend on running the actual app to catch it?

What exactly do you imagine us testing? This is a break between Http.Resilience (in dotnet/extensions) and gRPC client (which we don't own). .NET Aspire doesn't do anything with gRPC by default, we just happen to use it in one of our "playground apps".

ah, ok. nvm then!

@joperezr
Copy link
Member

joperezr commented Feb 6, 2024

FWIW, the issue will be fixed in extensions for 8.2, either by fixing the code that introduced the regression or by reverting the change entirely.

@eerhardt
Copy link
Member Author

eerhardt commented Feb 6, 2024

FWIW, the issue will be fixed in extensions for 8.2, either by fixing the code that introduced the regression or by reverting the change entirely.

We need to fix this sooner because our playground/eShopLite app is broken in main.

@eerhardt eerhardt enabled auto-merge (squash) February 6, 2024 23:42
@eerhardt eerhardt merged commit 94d3d26 into dotnet:main Feb 7, 2024
8 checks passed
radical pushed a commit to radical/aspire that referenced this pull request 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.
@eerhardt eerhardt deleted the RevertHttpResilienceUpgrade branch February 23, 2024 15:27
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-codeflow for labeling automated codeflow. intentionally a different color!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants