Feature/timeout for http client (#319)

* #318 http client obeys Qos timeout or defaults to 90 seconds, which is think is default for http client anyway but zero docs....

* #318 updated docs to specify default timeout and make it clear how to set it on a ReRoute basis

* #318 missed this

* #318 missed this

docs/features/qualityofservice.rst

@@ -17,6 +17,17 @@ Add the following section to a ReRoute configuration.
 
 You must set a number greater than 0 against ExceptionsAllowedBeforeBreaking for this rule to be 
 implemented. Duration of break is how long the circuit breaker will stay open for after it is tripped.
-TimeoutValue means ff a request takes more than 5 seconds it will automatically be timed out. 
+TimeoutValue means if a request takes more than 5 seconds it will automatically be timed out. 
 
-If you do not add a QoS section QoS will not be used.
+You can set the TimeoutValue in isoldation of the ExceptionsAllowedBeforeBreaking and DurationOfBreak options. 
+
+.. code-block:: json
+
+    "QoSOptions": {
+        "TimeoutValue":5000
+    }
+
+There is no point setting the other two in isolation as they affect each other :)
+
+If you do not add a QoS section QoS will not be used however Ocelot will default to a 90 second timeout 
+on all downstream requests. If someone needs this to be configurable open an issue.

src/Ocelot/Configuration/File/FileReRoute.cs

@@ -48,5 +48,6 @@ namespace Ocelot.Configuration.File
         public string Key { get;set; }
         public List<string> DelegatingHandlers {get;set;}
         public int Priority { get;set; }
+        public int Timeout { get; set; }
     }
 }

src/Ocelot/Configuration/QoSOptions.cs

@@ -16,12 +16,12 @@ namespace Ocelot.Configuration
             TimeoutStrategy = timeoutStrategy;
         }         
 
-        public int ExceptionsAllowedBeforeBreaking { get; private set; }
+        public int ExceptionsAllowedBeforeBreaking { get; }
 
-        public int DurationOfBreak { get; private set; }
+        public int DurationOfBreak { get; }
 
-        public int TimeoutValue { get; private set; }
+        public int TimeoutValue { get; }
 
-        public TimeoutStrategy TimeoutStrategy { get; private set; }
+        public TimeoutStrategy TimeoutStrategy { get; }
     }
 }

src/Ocelot/Requester/HttpClientBuilder.cs

@@ -17,6 +17,7 @@ namespace Ocelot.Requester
         private HttpClient _httpClient;
         private IHttpClient _client;
         private HttpClientHandler _httpclientHandler;
+        private readonly TimeSpan _defaultTimeout;
 
         public HttpClientBuilder(
             IDelegatingHandlerHandlerFactory factory, 
@@ -26,6 +27,10 @@ namespace Ocelot.Requester
             _factory = factory;
             _cacheHandlers = cacheHandlers;
             _logger = logger;
+
+            // This is hardcoded at the moment but can easily be added to configuration
+            // if required by a user request.
+            _defaultTimeout = TimeSpan.FromSeconds(90);
         }
 
         public IHttpClient Create(DownstreamContext request)
@@ -46,7 +51,14 @@ namespace Ocelot.Requester
                 CookieContainer = new CookieContainer()
             };
 
-            _httpClient = new HttpClient(CreateHttpMessageHandler(_httpclientHandler, request.DownstreamReRoute));
+            var timeout = request.DownstreamReRoute.QosOptionsOptions.TimeoutValue == 0
+                ? _defaultTimeout 
+                : TimeSpan.FromMilliseconds(request.DownstreamReRoute.QosOptionsOptions.TimeoutValue);
+
+            _httpClient = new HttpClient(CreateHttpMessageHandler(_httpclientHandler, request.DownstreamReRoute))
+            {
+                Timeout = timeout
+            };
 
             _client = new HttpClientWrapper(_httpClient);
 

src/Ocelot/Requester/HttpClientHttpRequester.cs

@@ -39,6 +39,10 @@ namespace Ocelot.Requester
             {
                 return new ErrorResponse<HttpResponseMessage>(new RequestTimedOutError(exception));
             }
+            catch (TaskCanceledException exception)
+            {
+                return new ErrorResponse<HttpResponseMessage>(new RequestTimedOutError(exception));
+            }
             catch (BrokenCircuitException exception)
             {
                 return new ErrorResponse<HttpResponseMessage>(new RequestTimedOutError(exception));

test/Ocelot.AcceptanceTests/ButterflyTracingTests.cs

@@ -14,6 +14,8 @@ using static Rafty.Infrastructure.Wait;
 
 namespace Ocelot.AcceptanceTests
 {
+    using Xunit.Abstractions;
+
     public class ButterflyTracingTests : IDisposable
     {
         private IWebHost _serviceOneBuilder;
@@ -23,9 +25,11 @@ namespace Ocelot.AcceptanceTests
         private string _downstreamPathOne;
         private string _downstreamPathTwo;
         private int _butterflyCalled;
+        private readonly ITestOutputHelper _output;
 
-        public ButterflyTracingTests()
+        public ButterflyTracingTests(ITestOutputHelper output)
         {
+            _output = output;
             _steps = new Steps();
         }
 
@@ -104,7 +108,9 @@ namespace Ocelot.AcceptanceTests
                 .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Tom"))
                 .BDDfy();
 
-            var commandOnAllStateMachines = WaitFor(10000).Until(() => _butterflyCalled == 4);
+            var commandOnAllStateMachines = WaitFor(10000).Until(() => _butterflyCalled >= 4);
+
+            _output.WriteLine($"_butterflyCalled is {_butterflyCalled}");
 
             commandOnAllStateMachines.ShouldBeTrue();
         }

test/Ocelot.AcceptanceTests/QoSTests.cs

@@ -25,6 +25,82 @@ namespace Ocelot.AcceptanceTests
             _steps = new Steps();
         }
 
+        [Fact]
+        public void should_not_timeout()
+        {
+            var configuration = new FileConfiguration
+            {
+                ReRoutes = new List<FileReRoute>
+                {
+                    new FileReRoute
+                    {
+                        DownstreamPathTemplate = "/",
+                        DownstreamHostAndPorts = new List<FileHostAndPort>
+                        {
+                            new FileHostAndPort
+                            {
+                                Host = "localhost",
+                                Port = 51569,
+                            }
+                        },
+                        DownstreamScheme = "http",
+                        UpstreamPathTemplate = "/",
+                        UpstreamHttpMethod = new List<string> { "Post" },
+                        QoSOptions = new FileQoSOptions
+                        {
+                            TimeoutValue = 1000,
+                        }
+                    }
+                }
+            };
+
+            this.Given(x => x.GivenThereIsAServiceRunningOn("http://localhost:51569", 200, string.Empty, 10))
+                .And(x => _steps.GivenThereIsAConfiguration(configuration))
+                .And(x => _steps.GivenOcelotIsRunning())
+                .And(x => _steps.GivenThePostHasContent("postContent"))
+                .When(x => _steps.WhenIPostUrlOnTheApiGateway("/"))
+                .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
+                .BDDfy();
+        }
+
+        [Fact]
+        public void should_timeout()
+        {
+            var configuration = new FileConfiguration
+            {
+                ReRoutes = new List<FileReRoute>
+                {
+                    new FileReRoute
+                    {
+                        DownstreamPathTemplate = "/",
+                        DownstreamHostAndPorts = new List<FileHostAndPort>
+                        {
+                            new FileHostAndPort
+                            {
+                                Host = "localhost",
+                                Port = 51579,
+                            }
+                        },
+                        DownstreamScheme = "http",
+                        UpstreamPathTemplate = "/",
+                        UpstreamHttpMethod = new List<string> { "Post" },
+                        QoSOptions = new FileQoSOptions
+                        {
+                            TimeoutValue = 10,
+                        }
+                    }
+                }
+            };
+
+            this.Given(x => x.GivenThereIsAServiceRunningOn("http://localhost:51579", 201, string.Empty, 1000))
+                .And(x => _steps.GivenThereIsAConfiguration(configuration))
+                .And(x => _steps.GivenOcelotIsRunning())
+                .And(x => _steps.GivenThePostHasContent("postContent"))
+                .When(x => _steps.WhenIPostUrlOnTheApiGateway("/"))
+                .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable))
+                .BDDfy();
+        }
+
         [Fact]
         public void should_open_circuit_breaker_then_close()
         {
@@ -122,7 +198,7 @@ namespace Ocelot.AcceptanceTests
             };
 
             this.Given(x => x.GivenThereIsAPossiblyBrokenServiceRunningOn("http://localhost:51872", "Hello from Laura"))
-                .And(x => x.GivenThereIsAServiceRunningOn("http://localhost:51880/", 200, "Hello from Tom"))
+                .And(x => x.GivenThereIsAServiceRunningOn("http://localhost:51880/", 200, "Hello from Tom", 0))
                 .And(x => _steps.GivenThereIsAConfiguration(configuration))
                 .And(x => _steps.GivenOcelotIsRunning())
                 .And(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
@@ -193,7 +269,7 @@ namespace Ocelot.AcceptanceTests
             _brokenService.Start();
         }
 
-        private void GivenThereIsAServiceRunningOn(string url, int statusCode, string responseBody)
+        private void GivenThereIsAServiceRunningOn(string url, int statusCode, string responseBody, int timeout)
         {
             _workingService = new WebHostBuilder()
                 .UseUrls(url)
@@ -205,6 +281,7 @@ namespace Ocelot.AcceptanceTests
                 {
                     app.Run(async context =>
                     {
+                        Thread.Sleep(timeout);
                         context.Response.StatusCode = statusCode;
                         await context.Response.WriteAsync(responseBody);
                     });

test/Ocelot.ManualTest/ocelot.json

@@ -1,5 +1,20 @@
 {
   "ReRoutes": [
+    {
+      "DownstreamPathTemplate": "/profile",
+      "DownstreamScheme": "http",
+      "UpstreamPathTemplate": "/profile",
+      "UpstreamHttpMethod": [ "Get" ],
+      "DownstreamHostAndPorts": [
+        {
+          "Host": "localhost",
+          "Port": 3000
+        }
+      ],
+      "QoSOptions": {
+        "TimeoutValue": 360000
+      }
+    },
     {
       "DownstreamPathTemplate": "/api/values",
       "DownstreamScheme": "http",

test/Ocelot.UnitTests/Requester/HttpClientBuilderTests.cs

@@ -50,6 +50,7 @@ namespace Ocelot.UnitTests.Requester
                 .WithIsQos(false)
                 .WithHttpHandlerOptions(new HttpHandlerOptions(false, false, false))
                 .WithReRouteKey("")
+                .WithQosOptions(new QoSOptionsBuilder().Build())
                 .Build();
 
             this.Given(x => GivenTheFactoryReturns())
@@ -66,6 +67,7 @@ namespace Ocelot.UnitTests.Requester
                 .WithIsQos(false)
                 .WithHttpHandlerOptions(new HttpHandlerOptions(false, false, false))
                 .WithReRouteKey("")
+                .WithQosOptions(new QoSOptionsBuilder().Build())
                 .Build();
 
             var fakeOne = new FakeDelegatingHandler();
@@ -93,6 +95,7 @@ namespace Ocelot.UnitTests.Requester
                 .WithIsQos(false)
                 .WithHttpHandlerOptions(new HttpHandlerOptions(false, true, false))
                 .WithReRouteKey("")
+                .WithQosOptions(new QoSOptionsBuilder().Build())
                 .Build();
 
             this.Given(_ => GivenADownstreamService())

test/Ocelot.UnitTests/Requester/HttpClientHttpRequesterTest.cs

@@ -21,7 +21,7 @@ namespace Ocelot.UnitTests.Requester
     public class HttpClientHttpRequesterTest
     {
         private readonly Mock<IHttpClientCache> _cacheHandlers;
-        private Mock<IDelegatingHandlerHandlerFactory> _factory;
+        private readonly Mock<IDelegatingHandlerHandlerFactory> _factory;
         private Response<HttpResponseMessage> _response;
         private readonly HttpClientHttpRequester _httpClientRequester;
         private DownstreamContext _request;
@@ -47,8 +47,12 @@ namespace Ocelot.UnitTests.Requester
         [Fact]
         public void should_call_request_correctly()
         {
-            var reRoute = new DownstreamReRouteBuilder().WithIsQos(false)
+            var reRoute = new DownstreamReRouteBuilder()
-                .WithHttpHandlerOptions(new HttpHandlerOptions(false, false, false)).WithReRouteKey("").Build();
+                .WithIsQos(false)
+                .WithHttpHandlerOptions(new HttpHandlerOptions(false, false, false))
+                .WithReRouteKey("")
+                .WithQosOptions(new QoSOptionsBuilder().Build())
+                .Build();
 
             var context = new DownstreamContext(new DefaultHttpContext())
             {
@@ -66,8 +70,12 @@ namespace Ocelot.UnitTests.Requester
         [Fact]
         public void should_call_request_unable_to_complete_request()
         {
-            var reRoute = new DownstreamReRouteBuilder().WithIsQos(false)
+            var reRoute = new DownstreamReRouteBuilder()
-                .WithHttpHandlerOptions(new HttpHandlerOptions(false, false, false)).WithReRouteKey("").Build();
+                .WithIsQos(false)
+                .WithHttpHandlerOptions(new HttpHandlerOptions(false, false, false))
+                .WithReRouteKey("")
+                .WithQosOptions(new QoSOptionsBuilder().Build())
+                .Build();
 
             var context = new DownstreamContext(new DefaultHttpContext())
             {
@@ -81,6 +89,30 @@ namespace Ocelot.UnitTests.Requester
                 .BDDfy();
         }
 
+        [Fact]
+        public void http_client_request_times_out()
+        {
+            var reRoute = new DownstreamReRouteBuilder()
+                .WithIsQos(false)
+                .WithHttpHandlerOptions(new HttpHandlerOptions(false, false, false))
+                .WithReRouteKey("")
+                .WithQosOptions(new QoSOptionsBuilder().WithTimeoutValue(1).Build())
+                .Build();
+
+            var context = new DownstreamContext(new DefaultHttpContext())
+            {
+                DownstreamReRoute = reRoute,
+                DownstreamRequest = new DownstreamRequest(new HttpRequestMessage() { RequestUri = new Uri("http://localhost:60080") }),
+            };
+
+            this.Given(_ => GivenTheRequestIs(context))
+                .And(_ => GivenTheHouseReturnsTimeoutHandler())
+                .When(_ => WhenIGetResponse())
+                .Then(_ => ThenTheResponseIsCalledError())
+                .And(_ => ThenTheErrorIsTimeout())
+                .BDDfy();
+        }
+
         private void GivenTheRequestIs(DownstreamContext request)
         {
             _request = request;            
@@ -101,6 +133,11 @@ namespace Ocelot.UnitTests.Requester
             _response.IsError.ShouldBeTrue();
         }
 
+        private void ThenTheErrorIsTimeout()
+        {
+            _response.Errors[0].ShouldBeOfType<RequestTimedOutError>();
+        }
+
         private void GivenTheHouseReturnsOkHandler()
         {
             var handlers = new List<Func<DelegatingHandler>>
@@ -111,6 +148,16 @@ namespace Ocelot.UnitTests.Requester
             _factory.Setup(x => x.Get(It.IsAny<DownstreamReRoute>())).Returns(new OkResponse<List<Func<DelegatingHandler>>>(handlers));
         }
 
+        private void GivenTheHouseReturnsTimeoutHandler()
+        {
+            var handlers = new List<Func<DelegatingHandler>>
+            {
+                () => new TimeoutDelegatingHandler()
+            };
+
+            _factory.Setup(x => x.Get(It.IsAny<DownstreamReRoute>())).Returns(new OkResponse<List<Func<DelegatingHandler>>>(handlers));
+        }
+
         class OkDelegatingHandler : DelegatingHandler
         {
             protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
@@ -118,5 +165,14 @@ namespace Ocelot.UnitTests.Requester
                 return Task.FromResult(new HttpResponseMessage());
             }
         }
+
+        class TimeoutDelegatingHandler : DelegatingHandler
+        {
+            protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
+            {
+                await Task.Delay(100000, cancellationToken);
+                return new HttpResponseMessage();
+            }
+        }
     }  
 }