From 0a66051b92ee4cd3f032fcbe59d0fbb56d33f3db Mon Sep 17 00:00:00 2001 From: Tom Gardham-Pallister Date: Mon, 6 Feb 2017 21:47:08 +0000 Subject: [PATCH] removed some code we dont need as not expecting any errors so should just throw an exception to the global handler --- .../Middleware/ExceptionHandlerMiddleware.cs | 4 ++-- .../LoadBalancer/LoadBalancers/ILoadBalancer.cs | 2 +- .../LeastConnectionLoadBalancer.cs | 4 +--- .../LoadBalancers/NoLoadBalancer.cs | 5 ++--- .../LoadBalancers/RoundRobinLoadBalancer.cs | 5 ++--- .../Middleware/LoadBalancingMiddleware.cs | 13 +++++++------ src/Ocelot/Responder/HttpContextResponder.cs | 6 ++---- src/Ocelot/Responder/IHttpResponder.cs | 5 ++--- .../Responder/Middleware/ResponderMiddleware.cs | 17 +++++------------ .../ConfigurationServiceProvider.cs | 2 +- .../LoadBalancer/LeastConnectionTests.cs | 2 +- .../LoadBalancer/LoadBalancerHouseTests.cs | 4 ++-- .../LoadBalancer/LoadBalancerMiddlewareTests.cs | 9 +++++++++ .../Responder/ResponderMiddlewareTests.cs | 8 -------- 14 files changed, 37 insertions(+), 49 deletions(-) diff --git a/src/Ocelot/Errors/Middleware/ExceptionHandlerMiddleware.cs b/src/Ocelot/Errors/Middleware/ExceptionHandlerMiddleware.cs index fc3ab200..6e6b511e 100644 --- a/src/Ocelot/Errors/Middleware/ExceptionHandlerMiddleware.cs +++ b/src/Ocelot/Errors/Middleware/ExceptionHandlerMiddleware.cs @@ -41,13 +41,13 @@ namespace Ocelot.Errors.Middleware var message = CreateMessage(context, e); _logger.LogError(message, e); - await SetInternalServerErrorOnResponse(context); + SetInternalServerErrorOnResponse(context); } _logger.LogDebug("ocelot pipeline finished"); } - private async Task SetInternalServerErrorOnResponse(HttpContext context) + private void SetInternalServerErrorOnResponse(HttpContext context) { context.Response.OnStarting(x => { diff --git a/src/Ocelot/LoadBalancer/LoadBalancers/ILoadBalancer.cs b/src/Ocelot/LoadBalancer/LoadBalancers/ILoadBalancer.cs index aa2a8f02..73d25d48 100644 --- a/src/Ocelot/LoadBalancer/LoadBalancers/ILoadBalancer.cs +++ b/src/Ocelot/LoadBalancer/LoadBalancers/ILoadBalancer.cs @@ -8,6 +8,6 @@ namespace Ocelot.LoadBalancer.LoadBalancers public interface ILoadBalancer { Task> Lease(); - Response Release(HostAndPort hostAndPort); + void Release(HostAndPort hostAndPort); } } \ No newline at end of file diff --git a/src/Ocelot/LoadBalancer/LoadBalancers/LeastConnectionLoadBalancer.cs b/src/Ocelot/LoadBalancer/LoadBalancers/LeastConnectionLoadBalancer.cs index bfb4817b..cd56ef91 100644 --- a/src/Ocelot/LoadBalancer/LoadBalancers/LeastConnectionLoadBalancer.cs +++ b/src/Ocelot/LoadBalancer/LoadBalancers/LeastConnectionLoadBalancer.cs @@ -53,7 +53,7 @@ namespace Ocelot.LoadBalancer.LoadBalancers } } - public Response Release(HostAndPort hostAndPort) + public void Release(HostAndPort hostAndPort) { lock(_syncLock) { @@ -69,8 +69,6 @@ namespace Ocelot.LoadBalancer.LoadBalancers _leases.Add(replacementLease); } } - - return new OkResponse(); } private Lease AddConnection(Lease lease) diff --git a/src/Ocelot/LoadBalancer/LoadBalancers/NoLoadBalancer.cs b/src/Ocelot/LoadBalancer/LoadBalancers/NoLoadBalancer.cs index f654dca8..bf66950b 100644 --- a/src/Ocelot/LoadBalancer/LoadBalancers/NoLoadBalancer.cs +++ b/src/Ocelot/LoadBalancer/LoadBalancers/NoLoadBalancer.cs @@ -17,13 +17,12 @@ namespace Ocelot.LoadBalancer.LoadBalancers public async Task> Lease() { - var service = _services.FirstOrDefault(); + var service = await Task.FromResult(_services.FirstOrDefault()); return new OkResponse(service.HostAndPort); } - public Response Release(HostAndPort hostAndPort) + public void Release(HostAndPort hostAndPort) { - return new OkResponse(); } } } diff --git a/src/Ocelot/LoadBalancer/LoadBalancers/RoundRobinLoadBalancer.cs b/src/Ocelot/LoadBalancer/LoadBalancers/RoundRobinLoadBalancer.cs index 0bb1f829..37efe22f 100644 --- a/src/Ocelot/LoadBalancer/LoadBalancers/RoundRobinLoadBalancer.cs +++ b/src/Ocelot/LoadBalancer/LoadBalancers/RoundRobinLoadBalancer.cs @@ -22,14 +22,13 @@ namespace Ocelot.LoadBalancer.LoadBalancers _last = 0; } - var next = _services[_last]; + var next = await Task.FromResult(_services[_last]); _last++; return new OkResponse(next.HostAndPort); } - public Response Release(HostAndPort hostAndPort) + public void Release(HostAndPort hostAndPort) { - return new OkResponse(); } } } diff --git a/src/Ocelot/LoadBalancer/Middleware/LoadBalancingMiddleware.cs b/src/Ocelot/LoadBalancer/Middleware/LoadBalancingMiddleware.cs index 7d09ea3a..ce37f828 100644 --- a/src/Ocelot/LoadBalancer/Middleware/LoadBalancingMiddleware.cs +++ b/src/Ocelot/LoadBalancer/Middleware/LoadBalancingMiddleware.cs @@ -31,14 +31,14 @@ namespace Ocelot.LoadBalancer.Middleware { _logger.LogDebug("started calling load balancing middleware"); - var getLoadBalancer = _loadBalancerHouse.Get(DownstreamRoute.ReRoute.LoadBalancerKey); - if(getLoadBalancer.IsError) + var loadBalancer = _loadBalancerHouse.Get(DownstreamRoute.ReRoute.LoadBalancerKey); + if(loadBalancer.IsError) { - SetPipelineError(getLoadBalancer.Errors); + SetPipelineError(loadBalancer.Errors); return; } - var hostAndPort = await getLoadBalancer.Data.Lease(); + var hostAndPort = await loadBalancer.Data.Lease(); if(hostAndPort.IsError) { SetPipelineError(hostAndPort.Errors); @@ -53,11 +53,12 @@ namespace Ocelot.LoadBalancer.Middleware { await _next.Invoke(context); - getLoadBalancer.Data.Release(hostAndPort.Data); + loadBalancer.Data.Release(hostAndPort.Data); } catch (Exception) { - getLoadBalancer.Data.Release(hostAndPort.Data); + loadBalancer.Data.Release(hostAndPort.Data); + _logger.LogDebug("error calling next middleware, exception will be thrown to global handler"); throw; } diff --git a/src/Ocelot/Responder/HttpContextResponder.cs b/src/Ocelot/Responder/HttpContextResponder.cs index 8b61e13b..40b60c30 100644 --- a/src/Ocelot/Responder/HttpContextResponder.cs +++ b/src/Ocelot/Responder/HttpContextResponder.cs @@ -24,7 +24,7 @@ namespace Ocelot.Responder _removeOutputHeaders = removeOutputHeaders; } - public async Task SetResponseOnHttpContext(HttpContext context, HttpResponseMessage response) + public async Task SetResponseOnHttpContext(HttpContext context, HttpResponseMessage response) { _removeOutputHeaders.Remove(response.Headers); @@ -56,7 +56,6 @@ namespace Ocelot.Responder { await stream.CopyToAsync(context.Response.Body); } - return new OkResponse(); } private static void AddHeaderIfDoesntExist(HttpContext context, KeyValuePair> httpResponseHeader) @@ -67,14 +66,13 @@ namespace Ocelot.Responder } } - public async Task SetErrorResponseOnContext(HttpContext context, int statusCode) + public void SetErrorResponseOnContext(HttpContext context, int statusCode) { context.Response.OnStarting(x => { context.Response.StatusCode = statusCode; return Task.CompletedTask; }, context); - return new OkResponse(); } } } \ No newline at end of file diff --git a/src/Ocelot/Responder/IHttpResponder.cs b/src/Ocelot/Responder/IHttpResponder.cs index 5292f4df..f885c673 100644 --- a/src/Ocelot/Responder/IHttpResponder.cs +++ b/src/Ocelot/Responder/IHttpResponder.cs @@ -1,13 +1,12 @@ using System.Net.Http; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; -using Ocelot.Responses; namespace Ocelot.Responder { public interface IHttpResponder { - Task SetResponseOnHttpContext(HttpContext context, HttpResponseMessage response); - Task SetErrorResponseOnContext(HttpContext context, int statusCode); + Task SetResponseOnHttpContext(HttpContext context, HttpResponseMessage response); + void SetErrorResponseOnContext(HttpContext context, int statusCode); } } diff --git a/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs b/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs index 06da92dc..6bce4ac6 100644 --- a/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs +++ b/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs @@ -46,34 +46,27 @@ namespace Ocelot.Responder.Middleware _logger.LogDebug("received errors setting error response"); - await SetErrorResponse(context, errors); + SetErrorResponse(context, errors); } else { _logger.LogDebug("no pipeline error, setting response"); - var setResponse = await _responder.SetResponseOnHttpContext(context, HttpResponseMessage); - - if (setResponse.IsError) - { - _logger.LogDebug("error setting response, returning error to client"); - - await SetErrorResponse(context, setResponse.Errors); - } + await _responder.SetResponseOnHttpContext(context, HttpResponseMessage); } } - private async Task SetErrorResponse(HttpContext context, List errors) + private void SetErrorResponse(HttpContext context, List errors) { var statusCode = _codeMapper.Map(errors); if (!statusCode.IsError) { - await _responder.SetErrorResponseOnContext(context, statusCode.Data); + _responder.SetErrorResponseOnContext(context, statusCode.Data); } else { - await _responder.SetErrorResponseOnContext(context, 500); + _responder.SetErrorResponseOnContext(context, 500); } } } diff --git a/src/Ocelot/ServiceDiscovery/ConfigurationServiceProvider.cs b/src/Ocelot/ServiceDiscovery/ConfigurationServiceProvider.cs index f6280d7b..98e7b0f6 100644 --- a/src/Ocelot/ServiceDiscovery/ConfigurationServiceProvider.cs +++ b/src/Ocelot/ServiceDiscovery/ConfigurationServiceProvider.cs @@ -15,7 +15,7 @@ namespace Ocelot.ServiceDiscovery public async Task> Get() { - return _services; + return await Task.FromResult(_services); } } } \ No newline at end of file diff --git a/test/Ocelot.UnitTests/LoadBalancer/LeastConnectionTests.cs b/test/Ocelot.UnitTests/LoadBalancer/LeastConnectionTests.cs index f5ea4738..07002ce3 100644 --- a/test/Ocelot.UnitTests/LoadBalancer/LeastConnectionTests.cs +++ b/test/Ocelot.UnitTests/LoadBalancer/LeastConnectionTests.cs @@ -51,7 +51,7 @@ namespace Ocelot.UnitTests.LoadBalancer { var hostAndPort = await _leastConnection.Lease(); await Task.Delay(_random.Next(1, 100)); - var response = _leastConnection.Release(hostAndPort.Data); + _leastConnection.Release(hostAndPort.Data); } [Fact] diff --git a/test/Ocelot.UnitTests/LoadBalancer/LoadBalancerHouseTests.cs b/test/Ocelot.UnitTests/LoadBalancer/LoadBalancerHouseTests.cs index 471e4b70..ac24b490 100644 --- a/test/Ocelot.UnitTests/LoadBalancer/LoadBalancerHouseTests.cs +++ b/test/Ocelot.UnitTests/LoadBalancer/LoadBalancerHouseTests.cs @@ -114,7 +114,7 @@ namespace Ocelot.UnitTests.LoadBalancer throw new NotImplementedException(); } - public Response Release(HostAndPort hostAndPort) + public void Release(HostAndPort hostAndPort) { throw new NotImplementedException(); } @@ -127,7 +127,7 @@ namespace Ocelot.UnitTests.LoadBalancer throw new NotImplementedException(); } - public Response Release(HostAndPort hostAndPort) + public void Release(HostAndPort hostAndPort) { throw new NotImplementedException(); } diff --git a/test/Ocelot.UnitTests/LoadBalancer/LoadBalancerMiddlewareTests.cs b/test/Ocelot.UnitTests/LoadBalancer/LoadBalancerMiddlewareTests.cs index 5d750f83..5a9eec87 100644 --- a/test/Ocelot.UnitTests/LoadBalancer/LoadBalancerMiddlewareTests.cs +++ b/test/Ocelot.UnitTests/LoadBalancer/LoadBalancerMiddlewareTests.cs @@ -170,6 +170,15 @@ namespace Ocelot.UnitTests.LoadBalancer .Verify(x => x.Add("OcelotMiddlewareErrors", _getLoadBalancerHouseError.Errors), Times.Once); } + private void ThenAnErrorSayingReleaseFailedIsSetOnThePipeline() + { + _scopedRepository + .Verify(x => x.Add("OcelotMiddlewareError", true), Times.Once); + + _scopedRepository + .Verify(x => x.Add("OcelotMiddlewareErrors", It.IsAny>()), Times.Once); + } + private void ThenAnErrorStatingHostAndPortCouldNotBeFoundIsSetOnPipeline() { _scopedRepository diff --git a/test/Ocelot.UnitTests/Responder/ResponderMiddlewareTests.cs b/test/Ocelot.UnitTests/Responder/ResponderMiddlewareTests.cs index b643028e..09a5c22c 100644 --- a/test/Ocelot.UnitTests/Responder/ResponderMiddlewareTests.cs +++ b/test/Ocelot.UnitTests/Responder/ResponderMiddlewareTests.cs @@ -62,19 +62,11 @@ namespace Ocelot.UnitTests.Responder { this.Given(x => x.GivenTheHttpResponseMessageIs(new HttpResponseMessage())) .And(x => x.GivenThereAreNoPipelineErrors()) - .And(x => x.GivenTheResponderReturns()) .When(x => x.WhenICallTheMiddleware()) .Then(x => x.ThenThereAreNoErrors()) .BDDfy(); } - private void GivenTheResponderReturns() - { - _responder - .Setup(x => x.SetResponseOnHttpContext(It.IsAny(), It.IsAny())) - .ReturnsAsync(new OkResponse()); - } - private void GivenThereAreNoPipelineErrors() { _scopedRepository