From 5e1605882be33a592a41fe08515a7cafbb67016d Mon Sep 17 00:00:00 2001 From: Tom Pallister Date: Wed, 18 Apr 2018 15:24:16 +0100 Subject: [PATCH] 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 | 15 +++- src/Ocelot/Configuration/File/FileReRoute.cs | 1 + src/Ocelot/Configuration/QoSOptions.cs | 8 +- src/Ocelot/Requester/HttpClientBuilder.cs | 14 +++- .../Requester/HttpClientHttpRequester.cs | 4 + .../Responder/ErrorsToHttpStatusCodeMapper.cs | 70 ++++++++-------- .../ButterflyTracingTests.cs | 10 ++- test/Ocelot.AcceptanceTests/QoSTests.cs | 81 ++++++++++++++++++- test/Ocelot.ManualTest/ocelot.json | 15 ++++ .../Requester/HttpClientBuilderTests.cs | 3 + .../Requester/HttpClientHttpRequesterTest.cs | 66 +++++++++++++-- 11 files changed, 236 insertions(+), 51 deletions(-) diff --git a/docs/features/qualityofservice.rst b/docs/features/qualityofservice.rst index 9eebf923..17bf373d 100644 --- a/docs/features/qualityofservice.rst +++ b/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. \ No newline at end of file +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. \ No newline at end of file diff --git a/src/Ocelot/Configuration/File/FileReRoute.cs b/src/Ocelot/Configuration/File/FileReRoute.cs index 0b2a06f9..5948c268 100644 --- a/src/Ocelot/Configuration/File/FileReRoute.cs +++ b/src/Ocelot/Configuration/File/FileReRoute.cs @@ -48,5 +48,6 @@ namespace Ocelot.Configuration.File public string Key { get;set; } public List DelegatingHandlers {get;set;} public int Priority { get;set; } + public int Timeout { get; set; } } } diff --git a/src/Ocelot/Configuration/QoSOptions.cs b/src/Ocelot/Configuration/QoSOptions.cs index 651bd506..8c5d6d27 100644 --- a/src/Ocelot/Configuration/QoSOptions.cs +++ b/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; } } } diff --git a/src/Ocelot/Requester/HttpClientBuilder.cs b/src/Ocelot/Requester/HttpClientBuilder.cs index 348bb207..fe80b11b 100644 --- a/src/Ocelot/Requester/HttpClientBuilder.cs +++ b/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); diff --git a/src/Ocelot/Requester/HttpClientHttpRequester.cs b/src/Ocelot/Requester/HttpClientHttpRequester.cs index ddbcf119..c7914c94 100644 --- a/src/Ocelot/Requester/HttpClientHttpRequester.cs +++ b/src/Ocelot/Requester/HttpClientHttpRequester.cs @@ -39,6 +39,10 @@ namespace Ocelot.Requester { return new ErrorResponse(new RequestTimedOutError(exception)); } + catch (TaskCanceledException exception) + { + return new ErrorResponse(new RequestTimedOutError(exception)); + } catch (BrokenCircuitException exception) { return new ErrorResponse(new RequestTimedOutError(exception)); diff --git a/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs b/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs index be7c59b9..61d27de0 100644 --- a/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs +++ b/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs @@ -1,38 +1,38 @@ -using System.Collections.Generic; -using System.Linq; -using Ocelot.Errors; - -namespace Ocelot.Responder -{ - public class ErrorsToHttpStatusCodeMapper : IErrorsToHttpStatusCodeMapper - { - public int Map(List errors) - { - if (errors.Any(e => e.Code == OcelotErrorCode.UnauthenticatedError)) - { - return 401; - } - - if (errors.Any(e => e.Code == OcelotErrorCode.UnauthorizedError - || e.Code == OcelotErrorCode.ClaimValueNotAuthorisedError - || e.Code == OcelotErrorCode.ScopeNotAuthorisedError - || e.Code == OcelotErrorCode.UserDoesNotHaveClaimError - || e.Code == OcelotErrorCode.CannotFindClaimError)) - { - return 403; - } - - if (errors.Any(e => e.Code == OcelotErrorCode.RequestTimedOutError)) - { - return 503; +using System.Collections.Generic; +using System.Linq; +using Ocelot.Errors; + +namespace Ocelot.Responder +{ + public class ErrorsToHttpStatusCodeMapper : IErrorsToHttpStatusCodeMapper + { + public int Map(List errors) + { + if (errors.Any(e => e.Code == OcelotErrorCode.UnauthenticatedError)) + { + return 401; + } + + if (errors.Any(e => e.Code == OcelotErrorCode.UnauthorizedError + || e.Code == OcelotErrorCode.ClaimValueNotAuthorisedError + || e.Code == OcelotErrorCode.ScopeNotAuthorisedError + || e.Code == OcelotErrorCode.UserDoesNotHaveClaimError + || e.Code == OcelotErrorCode.CannotFindClaimError)) + { + return 403; + } + + if (errors.Any(e => e.Code == OcelotErrorCode.RequestTimedOutError)) + { + return 503; + } + + if (errors.Any(e => e.Code == OcelotErrorCode.UnableToFindDownstreamRouteError)) + { + return 404; } - if (errors.Any(e => e.Code == OcelotErrorCode.UnableToFindDownstreamRouteError)) - { - return 404; - } - return 404; - } - } -} \ No newline at end of file + } + } +} diff --git a/test/Ocelot.AcceptanceTests/ButterflyTracingTests.cs b/test/Ocelot.AcceptanceTests/ButterflyTracingTests.cs index cde7b3bc..51f459ef 100644 --- a/test/Ocelot.AcceptanceTests/ButterflyTracingTests.cs +++ b/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(); } diff --git a/test/Ocelot.AcceptanceTests/QoSTests.cs b/test/Ocelot.AcceptanceTests/QoSTests.cs index 5e82ba3d..0a9a110a 100644 --- a/test/Ocelot.AcceptanceTests/QoSTests.cs +++ b/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 + { + new FileReRoute + { + DownstreamPathTemplate = "/", + DownstreamHostAndPorts = new List + { + new FileHostAndPort + { + Host = "localhost", + Port = 51569, + } + }, + DownstreamScheme = "http", + UpstreamPathTemplate = "/", + UpstreamHttpMethod = new List { "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 + { + new FileReRoute + { + DownstreamPathTemplate = "/", + DownstreamHostAndPorts = new List + { + new FileHostAndPort + { + Host = "localhost", + Port = 51579, + } + }, + DownstreamScheme = "http", + UpstreamPathTemplate = "/", + UpstreamHttpMethod = new List { "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); }); diff --git a/test/Ocelot.ManualTest/ocelot.json b/test/Ocelot.ManualTest/ocelot.json index 12f7f188..c21f8abb 100644 --- a/test/Ocelot.ManualTest/ocelot.json +++ b/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", diff --git a/test/Ocelot.UnitTests/Requester/HttpClientBuilderTests.cs b/test/Ocelot.UnitTests/Requester/HttpClientBuilderTests.cs index 45855f65..2f2bf1c7 100644 --- a/test/Ocelot.UnitTests/Requester/HttpClientBuilderTests.cs +++ b/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()) diff --git a/test/Ocelot.UnitTests/Requester/HttpClientHttpRequesterTest.cs b/test/Ocelot.UnitTests/Requester/HttpClientHttpRequesterTest.cs index c80ea391..d2210cf0 100644 --- a/test/Ocelot.UnitTests/Requester/HttpClientHttpRequesterTest.cs +++ b/test/Ocelot.UnitTests/Requester/HttpClientHttpRequesterTest.cs @@ -21,7 +21,7 @@ namespace Ocelot.UnitTests.Requester public class HttpClientHttpRequesterTest { private readonly Mock _cacheHandlers; - private Mock _factory; + private readonly Mock _factory; private Response _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) - .WithHttpHandlerOptions(new HttpHandlerOptions(false, false, false)).WithReRouteKey("").Build(); + var reRoute = new DownstreamReRouteBuilder() + .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) - .WithHttpHandlerOptions(new HttpHandlerOptions(false, false, false)).WithReRouteKey("").Build(); + var reRoute = new DownstreamReRouteBuilder() + .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(); + } + private void GivenTheHouseReturnsOkHandler() { var handlers = new List> @@ -111,6 +148,16 @@ namespace Ocelot.UnitTests.Requester _factory.Setup(x => x.Get(It.IsAny())).Returns(new OkResponse>>(handlers)); } + private void GivenTheHouseReturnsTimeoutHandler() + { + var handlers = new List> + { + () => new TimeoutDelegatingHandler() + }; + + _factory.Setup(x => x.Get(It.IsAny())).Returns(new OkResponse>>(handlers)); + } + class OkDelegatingHandler : DelegatingHandler { protected override Task 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 SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + await Task.Delay(100000, cancellationToken); + return new HttpResponseMessage(); + } + } } }