#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 <[email protected]>
Co-authored-by: Raman Maksimchuk <[email protected]>

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