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

HttpClient.Timeout overrides the TotalRequestTimeout when using AddStandardResilienceHandler #4770

Closed
martintmk opened this issue Nov 30, 2023 · 3 comments · Fixed by #4862 or #5363
Closed
Assignees
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug.

Comments

@martintmk
Copy link
Contributor

Description

The HttpClient.Timeout interferes with the TotalRequestTimeout when using HttpClient with standard pipeline. If HttpClient.Timeout is less than TotalRequestTimeout, then the TotalRequestTimeout is ignored.

This causes mis-alignment and unexpected behavior for anyone setting the TotalRequestTimeout to a value greater than 100 seconds, which is the default for HttpClient.Timeout.

Calling AddStandardResilienceHandler should disable the HttpClient.Timeout by setting the value to Timeout.InfiniteTimeSpan, because the overall timeout is already enforced by standard pipeline. This also saves some allocations due to HttpClient not needing to create CancellationTokenSource.

Reproduction Steps

Execute this code:

var client = new ServiceCollection()
    .AddHttpClient("my-client", c => c.BaseAddress = new Uri("https://jsonplaceholder.typicode.com"))
    .AddStandardResilienceHandler()
    .Configure(options =>
    {
        options.AttemptTimeout.Timeout = TimeSpan.FromSeconds(1);
        options.TotalRequestTimeout.Timeout = TimeSpan.FromSeconds(150);
        options.Retry.ShouldHandle = _ => new ValueTask<bool>(watch.Elapsed < TimeSpan.FromSeconds(100));
        options.Retry.MaxRetryAttempts = int.MaxValue;
    })
    .Services.BuildServiceProvider()
    .GetRequiredService<IHttpClientFactory>()
    .CreateClient("my-client");

await client.GetStringAsync("/", default);

This should be successful. Instead, the TaskCancelledException is thrown.

Expected behavior

Above example should not throw exception.

Actual behavior

Above example throws TaskCancelledException.

Regression?

No response

Known Workarounds

Manually disable HttpClient timeout:

var client = new ServiceCollection()
    .AddHttpClient("my-client", c =>
    {
        c.BaseAddress = new Uri("https://jsonplaceholder.typicode.com");
        c.Timeout = Timeout.InfiniteTimeSpan;
    })
    .AddStandardResilienceHandler();

Configuration

.NET SDK:
Version: 8.0.100
Commit: 57efcf1350
Workload version: 8.0.100-manifests.6a1e483a

Runtime Environment:
OS Name: Windows
OS Version: 10.0.22621
OS Platform: Windows
RID: win-x64
Base Path: C:\Program Files\dotnet\sdk\8.0.100\

Other information

No response

@martintmk martintmk added untriaged bug This issue describes a behavior which is not expected - a bug. labels Nov 30, 2023
@rafal-mz
Copy link
Contributor

rafal-mz commented Dec 1, 2023

Why is it better to use Polly's timeout policy implementation over built in HttpClient timeout? There is also a second possible solution to just make polly set the HttpClient.Timeout to the right value.

@martintmk
Copy link
Contributor Author

Why is it better to use Polly's timeout policy implementation over built in HttpClient timeout?

Few reasons why this is beneficial:

There is also a second possible solution to just make polly set the HttpClient.Timeout to the right value.

This would lead to race conditions between Polly's timeout and HttpClient's timeout. Sometimes the timeout would be triggered by HttpClient, other times by Polly.

@ghost ghost added the work in progress 🚧 label Jan 8, 2024
@ghost ghost removed the work in progress 🚧 label Jan 8, 2024
eerhardt added a commit to eerhardt/aspire that referenced this issue Feb 6, 2024
This has a fix for dotnet/extensions#4770 which causes our EndToEnd tests to timeout if a service doesn't start in 100 seconds.
eerhardt added a commit to dotnet/aspire that referenced this issue Feb 6, 2024
This has a fix for dotnet/extensions#4770 which causes our EndToEnd tests to timeout if a service doesn't start in 100 seconds.
@joperezr joperezr reopened this Feb 7, 2024
@iliar-turdushev iliar-turdushev self-assigned this Apr 29, 2024
@iliar-turdushev
Copy link
Contributor

iliar-turdushev commented Apr 29, 2024

The fix for current bug was reverted because it introduced the following bug #4924. When we fix #4924 we'll get back the fix for the current bug.

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 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
@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
6 participants