Skip to content

Shorten default timeout of individual calls to backend #657

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license.
//
using Azure.Core;
using Azure.Core.Pipeline;
using Azure.Data.AppConfiguration;
using Microsoft.Extensions.Configuration.AzureAppConfiguration.AzureKeyVault;
using Microsoft.Extensions.Configuration.AzureAppConfiguration.Extensions;
Expand All @@ -10,6 +11,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net.Http;
using System.Threading.Tasks;

namespace Microsoft.Extensions.Configuration.AzureAppConfiguration
Expand All @@ -18,10 +20,11 @@ namespace Microsoft.Extensions.Configuration.AzureAppConfiguration
/// Options used to configure the behavior of an Azure App Configuration provider.
/// If neither <see cref="Select"/> nor <see cref="SelectSnapshot"/> is ever called, all key-values with no label are included in the configuration provider.
/// </summary>
public class AzureAppConfigurationOptions
public class AzureAppConfigurationOptions : IDisposable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is dispose called on this in a customer app?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this, looks like we could call _options.Dispose in AzureAppConfigurationProvider.Dispose?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoever creates a disposable object should be the one to dispose it. In our case AzureAppConfigurationOptions is created internally and doesn't make sense to be disposed where it's created.

So I'm not sure we should be doing it this way at all. I'm leaning towards us doing this via a policy instead of passing an HttpClient.

{
private const int MaxRetries = 2;
private static readonly TimeSpan MaxRetryDelay = TimeSpan.FromMinutes(1);
private static readonly TimeSpan NetworkTimeout = TimeSpan.FromSeconds(10);
private static readonly KeyValueSelector DefaultQuery = new KeyValueSelector { KeyFilter = KeyFilter.Any, LabelFilter = LabelFilter.Null };

private List<KeyValueWatcher> _individualKvWatchers = new List<KeyValueWatcher>();
Expand All @@ -31,6 +34,10 @@ public class AzureAppConfigurationOptions
private List<KeyValueSelector> _selectors;
private IConfigurationRefresher _refresher = new AzureAppConfigurationRefresher();
private bool _selectCalled = false;
private HttpClientTransport _clientOptionsTransport = new HttpClientTransport(new HttpClient()
{
Timeout = NetworkTimeout
});

// The following set is sorted in descending order.
// Since multiple prefixes could start with the same characters, we need to trim the longest prefix first.
Expand Down Expand Up @@ -125,7 +132,7 @@ internal IEnumerable<IKeyValueAdapter> Adapters
/// <summary>
/// Options used to configure the client used to communicate with Azure App Configuration.
/// </summary>
internal ConfigurationClientOptions ClientOptions { get; } = GetDefaultClientOptions();
internal ConfigurationClientOptions ClientOptions { get; }

/// <summary>
/// Flag to indicate whether Key Vault options have been configured.
Expand Down Expand Up @@ -161,6 +168,8 @@ public AzureAppConfigurationOptions()

// Adds the default query to App Configuration if <see cref="Select"/> and <see cref="SelectSnapshot"/> are never called.
_selectors = new List<KeyValueSelector> { DefaultQuery };

ClientOptions = GetDefaultClientOptions();
}

/// <summary>
Expand Down Expand Up @@ -523,15 +532,27 @@ public AzureAppConfigurationOptions ConfigureStartupOptions(Action<StartupOption
return this;
}

private static ConfigurationClientOptions GetDefaultClientOptions()
private ConfigurationClientOptions GetDefaultClientOptions()
{
var clientOptions = new ConfigurationClientOptions(ConfigurationClientOptions.ServiceVersion.V2023_10_01);
clientOptions.Retry.MaxRetries = MaxRetries;
clientOptions.Retry.MaxDelay = MaxRetryDelay;
clientOptions.Retry.Mode = RetryMode.Exponential;
clientOptions.AddPolicy(new UserAgentHeaderPolicy(), HttpPipelinePosition.PerCall);
clientOptions.Transport = _clientOptionsTransport;

return clientOptions;
}

/// <summary>
/// Disposes of this instance of <see cref="AzureAppConfigurationOptions"/> and any resources it holds.
/// </summary>
public void Dispose()
{
if (_clientOptionsTransport != null)
{
_clientOptionsTransport.Dispose();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1232,6 +1232,13 @@ await ExecuteWithFailOverPolicyAsync<object>(clients, async (client) =>

private bool IsFailOverable(AggregateException ex)
{
TaskCanceledException tce = ex.InnerExceptions?.LastOrDefault(e => e is TaskCanceledException) as TaskCanceledException;

if (tce != null && tce.InnerException is TimeoutException)
{
return true;
}

RequestFailedException rfe = ex.InnerExceptions?.LastOrDefault(e => e is RequestFailedException) as RequestFailedException;

return rfe != null ? IsFailOverable(rfe) : false;
Expand Down