From ab79b01b41477274e5a20f4716ff3331138953b7 Mon Sep 17 00:00:00 2001 From: Alvin Huang Date: Fri, 8 Dec 2023 03:59:46 +0800 Subject: [PATCH 1/2] #1833 Default timeout(90s) of downstream requests is broken (#1834) * fix #1833, Default timeout(90s) of downstream requests is broken * Fix test Should_log_warning_if_downstream_service_returns_internal_server_error * Add unit tests * Recover mixed line endings. Add docs * Unit tests for HttpClientBuilder * Add issue info * Acceptance test * Just formatting --------- Co-authored-by: alvin huang Co-authored-by: Raman Maksimchuk --- .../Configuration/File/FileQoSOptions.cs | 25 +++++++---- test/Ocelot.AcceptanceTests/PollyQoSTests.cs | 44 ++++++++++++------- .../ReturnsErrorTests.cs | 2 +- .../FileModels/FileQoSOptionsTests.cs | 36 +++++++++++++++ .../Requester/HttpClientBuilderTests.cs | 29 +++++++++++- 5 files changed, 110 insertions(+), 26 deletions(-) create mode 100644 test/Ocelot.UnitTests/Configuration/FileModels/FileQoSOptionsTests.cs diff --git a/src/Ocelot/Configuration/File/FileQoSOptions.cs b/src/Ocelot/Configuration/File/FileQoSOptions.cs index 85b51fa43..85282707b 100644 --- a/src/Ocelot/Configuration/File/FileQoSOptions.cs +++ b/src/Ocelot/Configuration/File/FileQoSOptions.cs @@ -1,12 +1,19 @@ -namespace Ocelot.Configuration.File -{ - public class FileQoSOptions +namespace Ocelot.Configuration.File +{ + /// + /// File model for the "Quality of Service" feature options of the route. + /// + public class FileQoSOptions { + /// + /// Initializes a new instance of the class. + /// Default constructor. DON'T CHANGE!.. + /// public FileQoSOptions() { DurationOfBreak = 1; ExceptionsAllowedBeforeBreaking = 0; - TimeoutValue = int.MaxValue; + TimeoutValue = 0; } public FileQoSOptions(FileQoSOptions from) @@ -22,9 +29,9 @@ public FileQoSOptions(QoSOptions from) ExceptionsAllowedBeforeBreaking = from.ExceptionsAllowedBeforeBreaking; TimeoutValue = from.TimeoutValue; } - - public int DurationOfBreak { get; set; } - public int ExceptionsAllowedBeforeBreaking { get; set; } - public int TimeoutValue { get; set; } - } + + public int DurationOfBreak { get; set; } + public int ExceptionsAllowedBeforeBreaking { get; set; } + public int TimeoutValue { get; set; } + } } diff --git a/test/Ocelot.AcceptanceTests/PollyQoSTests.cs b/test/Ocelot.AcceptanceTests/PollyQoSTests.cs index f15838169..558ae7e3a 100644 --- a/test/Ocelot.AcceptanceTests/PollyQoSTests.cs +++ b/test/Ocelot.AcceptanceTests/PollyQoSTests.cs @@ -15,25 +15,25 @@ public PollyQoSTests() _steps = new Steps(); } - private static FileConfiguration FileConfigurationFactory(int port, QoSOptions options, - string httpMethod = nameof(HttpMethods.Get)) => new() - { - Routes = new List + private static FileConfiguration FileConfigurationFactory(int port, QoSOptions options, string httpMethod = nameof(HttpMethods.Get)) + => new() { - new() + Routes = new List { - DownstreamPathTemplate = "/", - DownstreamScheme = Uri.UriSchemeHttp, - DownstreamHostAndPorts = new() + new() { - new("localhost", port), + DownstreamPathTemplate = "/", + DownstreamScheme = Uri.UriSchemeHttp, + DownstreamHostAndPorts = new() + { + new("localhost", port), + }, + UpstreamPathTemplate = "/", + UpstreamHttpMethod = new() {httpMethod}, + QoSOptions = new FileQoSOptions(options), }, - UpstreamPathTemplate = "/", - UpstreamHttpMethod = new() {httpMethod}, - QoSOptions = new FileQoSOptions(options), }, - }, - }; + }; [Fact] public void Should_not_timeout() @@ -142,7 +142,21 @@ public void Open_circuit_should_not_effect_different_route() .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura")) .BDDfy(); } - + + [Fact(DisplayName = "1833: " + nameof(Should_timeout_per_default_after_90_seconds))] + public void Should_timeout_per_default_after_90_seconds() + { + var port = PortFinder.GetRandomPort(); + var configuration = FileConfigurationFactory(port, new QoSOptions(new FileQoSOptions()), HttpMethods.Get); + + this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}", 201, string.Empty, 95000)) + .And(x => _steps.GivenThereIsAConfiguration(configuration)) + .And(x => _steps.GivenOcelotIsRunningWithPolly()) + .When(x => _steps.WhenIGetUrlOnTheApiGateway("/")) + .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable)) + .BDDfy(); + } + private static void GivenIWaitMilliseconds(int ms) => Thread.Sleep(ms); private void GivenThereIsABrokenServiceRunningOn(string url) diff --git a/test/Ocelot.AcceptanceTests/ReturnsErrorTests.cs b/test/Ocelot.AcceptanceTests/ReturnsErrorTests.cs index 454b41455..c2157e24d 100644 --- a/test/Ocelot.AcceptanceTests/ReturnsErrorTests.cs +++ b/test/Ocelot.AcceptanceTests/ReturnsErrorTests.cs @@ -111,7 +111,7 @@ public void Should_log_warning_if_downstream_service_returns_internal_server_err .And(x => _steps.GivenThereIsAConfiguration(configuration)) .And(x => _steps.GivenOcelotIsRunningWithLogger()) .When(x => _steps.WhenIGetUrlOnTheApiGateway("/")) - .Then(x => _steps.ThenWarningShouldBeLogged(2)) + .Then(x => _steps.ThenWarningShouldBeLogged(1)) .BDDfy(); } diff --git a/test/Ocelot.UnitTests/Configuration/FileModels/FileQoSOptionsTests.cs b/test/Ocelot.UnitTests/Configuration/FileModels/FileQoSOptionsTests.cs new file mode 100644 index 000000000..003e1123f --- /dev/null +++ b/test/Ocelot.UnitTests/Configuration/FileModels/FileQoSOptionsTests.cs @@ -0,0 +1,36 @@ +using Ocelot.Configuration.File; + +namespace Ocelot.UnitTests.Configuration.FileModels; + +public class FileQoSOptionsTests +{ + [Fact(DisplayName = "1833: Default constructor must assign zero to the TimeoutValue property")] + public void Cstor_Default_AssignedZeroToTimeoutValue() + { + // Arrange, Act + var actual = new FileQoSOptions(); + + // Assert + Assert.Equal(0, actual.TimeoutValue); + } + + [Fact] + public void Cstor_Default_AssignedZeroToExceptionsAllowedBeforeBreaking() + { + // Arrange, Act + var actual = new FileQoSOptions(); + + // Assert + Assert.Equal(0, actual.ExceptionsAllowedBeforeBreaking); + } + + [Fact] + public void Cstor_Default_AssignedOneToDurationOfBreak() + { + // Arrange, Act + var actual = new FileQoSOptions(); + + // Assert + Assert.Equal(1, actual.DurationOfBreak); + } +} diff --git a/test/Ocelot.UnitTests/Requester/HttpClientBuilderTests.cs b/test/Ocelot.UnitTests/Requester/HttpClientBuilderTests.cs index e352ee4a3..a34b8af19 100644 --- a/test/Ocelot.UnitTests/Requester/HttpClientBuilderTests.cs +++ b/test/Ocelot.UnitTests/Requester/HttpClientBuilderTests.cs @@ -11,7 +11,7 @@ namespace Ocelot.UnitTests.Requester { - public class HttpClientBuilderTests : IDisposable + public sealed class HttpClientBuilderTests : IDisposable { private HttpClientBuilder _builder; private readonly Mock _factory; @@ -256,6 +256,33 @@ public void should_add_verb_to_cache_key(string verb) .BDDfy(); } + [Theory(DisplayName = "1833: " + nameof(Create_TimeoutValueInQosOptions_HttpClientTimeout))] + [InlineData(0, 90)] // default timeout is 90 seconds + [InlineData(20, 20)] // QoS timeout + public void Create_TimeoutValueInQosOptions_HttpClientTimeout(int qosTimeout, int expectedSeconds) + { + // Arrange + var qosOptions = new QoSOptionsBuilder() + .WithTimeoutValue(qosTimeout * 1000) + .Build(); + var handlerOptions = new HttpHandlerOptionsBuilder() + .WithUseMaxConnectionPerServer(int.MaxValue) + .Build(); + var route = new DownstreamRouteBuilder() + .WithQosOptions(qosOptions) + .WithHttpHandlerOptions(handlerOptions) + .Build(); + GivenTheFactoryReturnsNothing(); + + // Act + var actualClient = _builder.Create(route); + + // Assert + var actual = actualClient?.Client?.Timeout; + Assert.NotNull(actual); + Assert.Equal(expectedSeconds, actual.Value.TotalSeconds); + } + private void GivenARealCache() { _realCache = new MemoryHttpClientCache(); From 37265ad46e87cd6299fc68dbf3b00ba36a6402a6 Mon Sep 17 00:00:00 2001 From: Raman Maksimchuk Date: Fri, 8 Dec 2023 11:46:18 +0300 Subject: [PATCH 2/2] Release 22.0.1 --- ReleaseNotes.md | 47 +++++++++++++++++------------------------------ build.cake | 8 ++++---- 2 files changed, 21 insertions(+), 34 deletions(-) diff --git a/ReleaseNotes.md b/ReleaseNotes.md index e3df12c17..460241423 100644 --- a/ReleaseNotes.md +++ b/ReleaseNotes.md @@ -1,33 +1,20 @@ -## October 2023 (version {0}) aka [Swiss Locomotive](https://en.wikipedia.org/wiki/SBB-CFF-FFS_Ae_6/6) release -> Codenamed as **[Swiss Locomotive](https://www.google.com/search?q=swiss+locomotive)** +## Hotfix release (version {0}) +> Default timeout vs the [Quality of Service](https://ocelot.readthedocs.io/en/latest/features/qualityofservice.html) feature -### Focused On -
- Logging feature. Performance review, redesign and improvements with new best practices to log +Special thanks to **Alvin Huang**, @huanguolin! - - Proposing a centralized `WriteLog` method for the `OcelotLogger` - - Factory methods for computed strings such as `string.Format` or interpolated strings - - Using `ILogger.IsEnabled` before calling the native `WriteLog` implementation and invoking string factory method -
-
- Quality of Service feature. Redesign and stabilization, and it produces less log records now. - - - Fixing issue with [Polly](https://www.thepollyproject.org/) Circuit Breaker not opening after max number of retries reached - - Removing useless log calls that could have an impact on performance - - Polly [lib](https://www.nuget.org/packages/Polly#versions-body-tab) reference updating to latest `8.2.0` with some code improvements -
-
- Documentation for Logging, Request ID, Routing and Websockets - - - [Logging](https://ocelot.readthedocs.io/en/latest/features/logging.html) - - [Request ID](https://ocelot.readthedocs.io/en/latest/features/requestid.html) - - [Routing](https://ocelot.readthedocs.io/en/latest/features/routing.html) - - [Websockets](https://ocelot.readthedocs.io/en/latest/features/websockets.html) -
-
- Testing improvements and stabilization aka bug fixing +### About +The bug is related to the **Quality of Service** feature (aka **QoS**) and the [HttpClient.Timeout](https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.timeout) property. +- If JSON `QoSOptions` section is defined in the route config, then the bug is masked rather than active, and the timeout value is assigned from the [QoS TimeoutValue](https://ocelot.readthedocs.io/en/latest/features/qualityofservice.html#quality-of-service:~:text=%22TimeoutValue%22%3A%205000) property. +- If the `QoSOptions` section **is not** defined in the route config or the [TimeoutValue](https://ocelot.readthedocs.io/en/latest/features/qualityofservice.html#quality-of-service:~:text=%22TimeoutValue%22%3A%205000) property is missing, then the bug is **active** and affects downstream requests that **never time out**. - - [Routing](https://ocelot.readthedocs.io/en/latest/features/routing.html) bug fixing: query string placeholders including **CatchAll** one aka `{{everything}}` and query string duplicates removal - - [QoS](https://ocelot.readthedocs.io/en/latest/features/qualityofservice.html) bug fixing: Polly circuit breaker exceptions - - Testing bug fixing: rare failed builds because of unstable Polly tests. Acceptance common logic for ports -
+### Technical info +In version [22.0](https://github.com/ThreeMammals/Ocelot/releases/tag/22.0.0), the bug was found in the explicit default constructor of the [FileQoSOptions](https://github.com/ThreeMammals/Ocelot/blob/main/src/Ocelot/Configuration/File/FileQoSOptions.cs) class with a maximum [TimeoutValue](https://github.com/ThreeMammals/Ocelot/blob/main/src/Ocelot/Configuration/File/FileQoSOptions.cs#L9). Previously, the default constructor was implicit with the default assignment of zero `0` to all `int` properties. + +The new explicit default constructor breaks the old implementation of [QoS TimeoutValue](https://github.com/ThreeMammals/Ocelot/blob/main/src/Ocelot/Requester/HttpClientBuilder.cs#L53-L55) logic, as our [QoS documentation](https://ocelot.readthedocs.io/en/latest/features/qualityofservice.html#quality-of-service:~:text=If%20you%20do%20not%20add%20a%20QoS%20section%2C%20QoS%20will%20not%20be%20used%2C%20however%20Ocelot%20will%20default%20to%20a%2090%20seconds%20timeout%20on%20all%20downstream%20requests.) states: +![image](https://github.com/ThreeMammals/Ocelot/assets/12430413/2c6b2cd3-e1c6-4510-9e46-883468c140ec)
+**Finally**, the "default 90 second" logic for `HttpClient` breaks down when there are no **QoS** options and all requests on those routes are infinite, if, for example, downstream services are down or stuck. + +#### The Bug Artifacts +- Reported bug issue: [1833](https://github.com/ThreeMammals/Ocelot/issues/1833) by @huanguolin +- Hotfix PR: [1834](https://github.com/ThreeMammals/Ocelot/pull/1834) by @huanguolin diff --git a/build.cake b/build.cake index 1e8f4a032..2b1c99828 100644 --- a/build.cake +++ b/build.cake @@ -512,10 +512,10 @@ Task("PublishToNuget") .Does(() => { Information("Skipping of publishing to NuGet..."); - // if (IsRunningOnCircleCI()) - // { - // PublishPackages(packagesDir, artifactsFile, nugetFeedStableKey, nugetFeedStableUploadUrl, nugetFeedStableSymbolsUploadUrl); - // } + if (IsRunningOnCircleCI()) + { + PublishPackages(packagesDir, artifactsFile, nugetFeedStableKey, nugetFeedStableUploadUrl, nugetFeedStableSymbolsUploadUrl); + } }); RunTarget(target);