Merge pull request #1847 from ThreeMammals/release/22.0

Release 22.0.1 - Hotfix

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
-<details>
-  <summary><b>Logging feature</b>. Performance review, redesign and improvements with new best practices to log</summary>
+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
-</details>
-<details>
-  <summary><b>Quality of Service feature</b>. Redesign and stabilization, and it produces less log records now.</summary>
-
-  - 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
-</details>
-<details>
-  <summary>Documentation for <b>Logging</b>, <b>Request ID</b>, <b>Routing</b> and <b>Websockets</b></summary>
-
-  - [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)
-</details>
-<details>
-  <summary>Testing improvements and stabilization aka <b>bug fixing</b></summary>
+### 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
-</details>
+### 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) <br/>
+**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

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);

src/Ocelot/Configuration/File/FileQoSOptions.cs

@@ -1,12 +1,19 @@
-namespace Ocelot.Configuration.File
-{
-    public class FileQoSOptions
+namespace Ocelot.Configuration.File
+{
+    /// <summary>
+    /// File model for the "Quality of Service" feature options of the route.
+    /// </summary>
+    public class FileQoSOptions
     {
+        /// <summary>
+        /// Initializes a new instance of the <see cref="FileQoSOptions"/> class.
+        /// <para>Default constructor. DON'T CHANGE!..</para>
+        /// </summary>
         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; }
+    }
 }

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<FileRoute>
+        private static FileConfiguration FileConfigurationFactory(int port, QoSOptions options, string httpMethod = nameof(HttpMethods.Get))
+            => new()
             {
-                new()
+                Routes = new List<FileRoute>
                 {
-                    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)

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();
         }
 

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);
+    }
+}

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<IDelegatingHandlerHandlerFactory> _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();