Disable HttpClient's timeout for Standard Resilience and Hedging (#5363)

* 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
dotnet · Sep 9, 2024 · 2739017 · 2739017
1 parent 858624b
commit 2739017
Show file tree

Hide file tree

Showing 9 changed files with 152 additions and 4 deletions.
diff --git a/eng/MSBuild/Packaging.targets b/eng/MSBuild/Packaging.targets
@@ -11,15 +11,17 @@
   <PropertyGroup>
     <IsPackable Condition="'$(Stage)' == ''">false</IsPackable>
     <BeforePack>$(BeforePack);_VerifyMinimumSupportedTfmForPackagingIsUsed;_AddNETStandardCompatErrorFileForPackaging</BeforePack>
+    <IsPackNet462 Condition="'$(MinimumSupportedTfmForPackaging)' != '' and
+                             '$(ConditionalNet462)' != '' and
+                             ($(TargetFrameworks.Contains('$(MinimumSupportedTfmForPackaging)')) and $(TargetFrameworks.Contains('$(ConditionalNet462)')))"
+                  >true</IsPackNet462>
   </PropertyGroup>
 
   <!-- Include a compat warning for users trying to use our packages on unsupported TFMs -->
   <ItemGroup>
     <NETStandardCompatError Include="$(ConditionalNet462)"
                             Supported="$(MinimumSupportedTfmForPackaging)"
-                            Condition="'$(MinimumSupportedTfmForPackaging)' != '' and
-                                       '$(ConditionalNet462)' != '' and
-                                       ($(TargetFrameworks.Contains('$(MinimumSupportedTfmForPackaging)')) and $(TargetFrameworks.Contains('$(ConditionalNet462)')))" />
+                            Condition="'$(IsPackNet462)' == 'true'" />
   </ItemGroup>
 
   <PropertyGroup Condition=" '$(IsPackable)' == 'true' and '$(IsShipping)' == 'true' ">

diff --git a/eng/packages/TestOnly.props b/eng/packages/TestOnly.props
@@ -5,7 +5,7 @@
     <PackageVersion Include="autofixture" Version="4.17.0" />
     <PackageVersion Include="BenchmarkDotNet" Version="0.13.5" />
     <PackageVersion Include="FluentAssertions" Version="6.11.0" />
-    <PackageVersion Include="Grpc.AspNetCore" Version="2.57.0" />
+    <PackageVersion Include="Grpc.AspNetCore" Version="2.65.0" />
     <PackageVersion Include="Microsoft.Diagnostics.Tracing.TraceEvent" Version="3.1.3" />
     <PackageVersion Include="Moq.AutoMock" Version="3.1.0" />
     <PackageVersion Include="Moq" Version="4.18.4" />

diff --git a/...osoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs b/...osoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Net.Http;
+using System.Threading;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.DependencyInjection.Extensions;
 using Microsoft.Extensions.Http.Resilience;
@@ -139,6 +140,9 @@ public static IStandardHedgingHandlerBuilder AddStandardHedgingHandler(this IHtt
             })
             .SelectPipelineByAuthority();
 
+        // Disable the HttpClient timeout to allow the timeout strategies to control the timeout.
+        _ = builder.ConfigureHttpClient(client => client.Timeout = Timeout.InfiniteTimeSpan);
+
         return new StandardHedgingHandlerBuilder(builder.Name, builder.Services, routingBuilder);
     }
 

diff --git a/...ibraries/Microsoft.Extensions.Http.Resilience/Microsoft.Extensions.Http.Resilience.csproj b/...ibraries/Microsoft.Extensions.Http.Resilience/Microsoft.Extensions.Http.Resilience.csproj
@@ -38,4 +38,39 @@
     <InternalsVisibleToTest Include="$(AssemblyName).Tests" />
     <InternalsVisibleToDynamicProxyGenAssembly2 Include="*" />
   </ItemGroup>
+
+  <ItemGroup>
+    <None Include="buildTransitive\**\*"
+          CopyToOutputDirectory="PreserveNewest"
+          Pack="true"
+          PackagePath="buildTransitive\$(MinimumSupportedTfmForPackaging)\"
+          Condition="'$(MinimumSupportedTfmForPackaging)' != '' and $(TargetFrameworks.Contains('$(MinimumSupportedTfmForPackaging)'))" />
+
+    <!-- For net462 we copy all buildTransitive files except the .targets file. We have to rename the .targets file
+         when copying it because our build process automatically adds a .targets file with the same name as the project
+         file. Then we'll import the renamed .targets file in the automatically added .targets file. For that we need
+         to set the _AdditionalNETStandardCompatErrorFileContents variable. -->
+    <None Include="buildTransitive\**\*"
+          Exclude="buildTransitive\$(MSBuildProjectName).targets"
+          CopyToOutputDirectory="PreserveNewest"
+          Pack="true"
+          PackagePath="buildTransitive\net462\"
+          Condition="'$(IsPackNet462)' == 'true'" />
+
+    <None Include="buildTransitive\$(MSBuildProjectName).targets"
+          CopyToOutputDirectory="PreserveNewest"
+          Pack="true"
+          PackagePath="buildTransitive\net462\$(MSBuildProjectName).net462.targets"
+          Condition="'$(IsPackNet462)' == 'true'" />
+  </ItemGroup>
+
+  <!-- For net462 we automatically add the .targets file, and to include the code that checks the version of the
+       Grpc.Net.ClientFactory package we need to set the _AdditionalNETStandardCompatErrorFileContents variable. -->
+  <PropertyGroup>
+    <_AdditionalNETStandardCompatErrorFileContents>
+      <![CDATA[
+  <Import Project="%24(MSBuildThisFileDirectory)\$(MSBuildProjectName).net462.targets" />
+]]>
+    </_AdditionalNETStandardCompatErrorFileContents>
+  </PropertyGroup>
 </Project>
diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/README.md b/src/Libraries/Microsoft.Extensions.Http.Resilience/README.md
@@ -73,6 +73,36 @@ clientBuilder.AddResilienceHandler("myHandler", b =>
 });
 ```
 
+## Known issues
+
+The following sections detail various known issues.
+
+### Compatibility with the `Grpc.Net.ClientFactory` package
+
+If you're using `Grpc.Net.ClientFactory` version `2.63.0` or earlier, then enabling the standard resilience or hedging handlers for a gRPC client could cause a runtime exception. Specifically, consider the following code sample:
+
+```csharp
+services
+    .AddGrpcClient<Greeter.GreeterClient>()
+    .AddStandardResilienceHandler();
+```
+
+The preceding code results in the following exception:
+
+```Output
+System.InvalidOperationException: The ConfigureHttpClient method is not supported when creating gRPC clients. Unable to create client with name 'GreeterClient'.
+```
+
+To resolve this issue, we recommend upgrading to `Grpc.Net.ClientFactory` version `2.64.0` or later.
+
+There's a build time check that verifies if you're using `Grpc.Net.ClientFactory` version `2.63.0` or earlier, and if you are the check produces a compilation warning. You can suppress the warning by setting the following property in your project file:
+
+```xml
+<PropertyGroup>
+  <SuppressCheckGrpcNetClientFactoryVersion>true</SuppressCheckGrpcNetClientFactoryVersion>
+</PropertyGroup>
+```
+
 ## Feedback & Contributing
 
 We welcome feedback and contributions in [our GitHub repo](https://github.com/dotnet/extensions).
diff --git a/...ns.Http.Resilience/Resilience/ResilienceHttpClientBuilderExtensions.StandardResilience.cs b/...ns.Http.Resilience/Resilience/ResilienceHttpClientBuilderExtensions.StandardResilience.cs
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Threading;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Http.Resilience;
@@ -86,6 +87,9 @@ public static IHttpStandardResiliencePipelineBuilder AddStandardResilienceHandle
                 .AddTimeout(options.AttemptTimeout);
         });
 
+        // Disable the HttpClient timeout to allow the timeout strategies to control the timeout.
+        _ = builder.ConfigureHttpClient(client => client.Timeout = Timeout.InfiniteTimeSpan);
+
         return new HttpStandardResiliencePipelineBuilder(optionsName, builder.Services);
     }
 

diff --git a/...t.Extensions.Http.Resilience/buildTransitive/Microsoft.Extensions.Http.Resilience.targets b/...t.Extensions.Http.Resilience/buildTransitive/Microsoft.Extensions.Http.Resilience.targets
@@ -0,0 +1,54 @@
+<Project>
+  <PropertyGroup>
+    <_GrpcNetClientFactory>Grpc.Net.ClientFactory</_GrpcNetClientFactory>
+    <_CompatibleGrpcNetClientFactoryVersion>2.64.0</_CompatibleGrpcNetClientFactoryVersion>
+    <_GrpcNetClientFactoryVersionIsIncorrect>Grpc.Net.ClientFactory 2.63.0 or earlier could cause issues when used together with Microsoft.Extensions.Http.Resilience. For more details, see https://learn.microsoft.com/dotnet/core/resilience/http-resilience#known-issues. Consider using Grpc.Net.ClientFactory $(_CompatibleGrpcNetClientFactoryVersion) or later. To suppress the warning set SuppressCheckGrpcNetClientFactoryVersion=true.</_GrpcNetClientFactoryVersionIsIncorrect>
+  </PropertyGroup>
+
+  <!--
+    Check whether the project is referencing Grpc.Net.ClientFactory 2.64.0 or later.
+    If the vesion is earlier warn the user to update.
+    See https://learn.microsoft.com/en-us/dotnet/core/resilience/http-resilience#known-issues for more details.
+    -->
+  <Target Name="_CheckGrpcNetClientFactoryVersion"
+          BeforeTargets="ResolveReferences"
+          Condition=" '$(SuppressCheckGrpcNetClientFactoryVersion)' != 'true' ">
+    <ItemGroup>
+      <!-- Find the package in the .csproj file. -->
+      <_GrpcNetClientFactoryPackageReference Include="@(PackageReference)" Condition=" '%(PackageReference.Identity)' == '$(_GrpcNetClientFactory)' " />
+
+      <!-- Find the version of the package in the Central Package Source. The solution uses the Central Package Management. -->
+      <_GrpcNetClientFactoryPackageVersion Include="@(PackageVersion)" Condition=" '%(PackageVersion.Identity)' == '$(_GrpcNetClientFactory)' " />
+
+      <!-- The package is added to the project as a transitive dependency. -->
+      <_GrpcNetClientFactoryTransitiveDependency Include="@(ReferencePath)" Condition=" '%(ReferencePath.NuGetPackageId)' == '$(_GrpcNetClientFactory)' " />
+    </ItemGroup>
+
+    <!-- The version of the package is included in the .csproj file. -->
+    <Warning Condition=" @(_GrpcNetClientFactoryPackageReference->Count()) &gt; 0
+                         AND '%(_GrpcNetClientFactoryPackageReference.Version)' != ''
+                         AND $([MSBuild]::VersionLessThan('%(_GrpcNetClientFactoryPackageReference.Version)', '$(_CompatibleGrpcNetClientFactoryVersion)')) "
+             Text="$(_GrpcNetClientFactoryVersionIsIncorrect)" />
+
+    <!-- The solution uses the Central Package Management and the version of the package is overridden in the .csproj file using the VersionOverride property. -->
+    <Warning Condition=" '$(ManagePackageVersionsCentrally)' == 'true'
+                         AND @(_GrpcNetClientFactoryPackageReference->Count()) &gt; 0
+                         AND '%(_GrpcNetClientFactoryPackageReference.VersionOverride)' != ''
+                         AND $([MSBuild]::VersionLessThan('%(_GrpcNetClientFactoryPackageReference.VersionOverride)', '$(_CompatibleGrpcNetClientFactoryVersion)')) "
+             Text="$(_GrpcNetClientFactoryVersionIsIncorrect)" />
+
+    <!-- The solution uses the Central Package Management and the version of the package is included in the Central Package Source. -->
+    <Warning Condition=" '$(ManagePackageVersionsCentrally)' == 'true'
+                         AND @(_GrpcNetClientFactoryPackageReference->Count()) &gt; 0
+                         AND '%(_GrpcNetClientFactoryPackageVersion.Version)' != ''
+                         AND $([MSBuild]::VersionLessThan('%(_GrpcNetClientFactoryPackageVersion.Version)', '$(_CompatibleGrpcNetClientFactoryVersion)')) "
+             Text="$(_GrpcNetClientFactoryVersionIsIncorrect)" />
+
+    <!-- This condition handles a case when the package is added to the project as a transitive dependency. -->
+    <Warning Condition=" @(_GrpcNetClientFactoryPackageReference->Count()) == 0
+                         AND @(_GrpcNetClientFactoryTransitiveDependency->Count()) &gt; 0
+                         AND '%(_GrpcNetClientFactoryTransitiveDependency.NuGetPackageVersion)' != ''
+                         AND $([MSBuild]::VersionLessThan('%(_GrpcNetClientFactoryTransitiveDependency.NuGetPackageVersion)', '$(_CompatibleGrpcNetClientFactoryVersion)')) "
+             Text="$(_GrpcNetClientFactoryVersionIsIncorrect)" />
+  </Target>
+</Project>
diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs
@@ -265,6 +265,14 @@ public async Task DynamicReloads_Ok(bool asynchronous = true)
         AssertNoResponse();
     }
 
+    [Fact]
+    public void AddStandardResilienceHandler_EnsureHttpClientTimeoutDisabled()
+    {
+        var client = CreateClientWithHandler();
+
+        client.Timeout.Should().Be(Timeout.InfiniteTimeSpan);
+    }
+
     [Theory]
 #if NET6_0_OR_GREATER
     [CombinatorialData]

diff --git a/....Extensions.Http.Resilience.Tests/Resilience/HttpClientBuilderExtensionsTests.Standard.cs b/....Extensions.Http.Resilience.Tests/Resilience/HttpClientBuilderExtensionsTests.Standard.cs
@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Net;
 using System.Net.Http;
+using System.Threading;
 using System.Threading.Tasks;
 using FluentAssertions;
 using Microsoft.Extensions.Configuration;
@@ -257,6 +258,16 @@ public async Task DynamicReloads_Ok(bool asynchronous = true)
         requests.Should().HaveCount(11);
     }
 
+    [Fact]
+    public void AddStandardResilienceHandler_EnsureHttpClientTimeoutDisabled()
+    {
+        var builder = new ServiceCollection().AddLogging().AddMetrics().AddHttpClient("test").AddStandardResilienceHandler();
+
+        using var client = builder.Services.BuildServiceProvider().GetRequiredService<IHttpClientFactory>().CreateClient("test");
+
+        client.Timeout.Should().Be(Timeout.InfiniteTimeSpan);
+    }
+
     private static void AddStandardResilienceHandler(
         MethodArgs mode,
         IHttpClientBuilder builder,