#356 #695 #1924 Custom HttpMessageInvoker pooling (#1824)

* first version

* first working version of the new http client pool

* Some cleanup, removing classes that aren't used

* some more cleanup

* forgot passing PooledConnectionLifetime

* adding todo for connection pool and request timeouts

* some code cleanup

* ongoing process refactoring tests

* a little mistake with big effects

* Several refactorings, disposing http response message to ensure that the connection is freed. Moving errors from Polly provider to Errors\QoS.

* providing some comments

* adding response body benchmark

* some minor changes in MessageInvokerPool.

* using context.RequestAborted in responder middleware (copying the response body from downstream service to the http context)

* Fix style warnings

* code review

* moving response.Content.ReadAsStreamAsync nearer to CopyToAsync with using.
Making sure, that the content stream is disposed

* HttpResponse.Content never returns null (from .net 5 onwards)

* adding more unit tests (validating, log warning if passthrough certificate, cookies are returned, message invoker timeout)

* adding a tolerance margin

* adding new acceptance test, checking memory usage. Needs to be compared with current implementation first.

* Review tests by @raman-m

* Update src/Ocelot/Configuration/HttpHandlerOptions.cs

* Update src/Ocelot/Middleware/DownstreamResponse.cs

* Update src/Ocelot/Requester/MessageInvokerPool.cs

* Update src/Ocelot/Requester/HttpExceptionToErrorMapper.cs

* Update src/Ocelot/Responder/HttpContextResponder.cs

* Update src/Ocelot/Middleware/DownstreamResponse.cs

* Use null-coalescing operator for `Nullable<int>` obj

* some modifications in the acceptance test, adding a tolerance of 1 Mb

* finalizing content tests. Making sure, that the downstream response body is not copied by the api gateway.

* adapting tolerances

* Disable StyleCop rule SA1010 which is in conflict with collection initialization block vs whitespace.
More: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1010.md

* Refactor Content tests

---------

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

codeanalysis.ruleset

@@ -18,6 +18,7 @@
     <Rule Id="SA1005" Action="None" />
     <Rule Id="SA1008" Action="None" />
     <Rule Id="SA1009" Action="None" />
+    <Rule Id="SA1010" Action="None" />
     <Rule Id="SA1011" Action="None" />
     <Rule Id="SA1012" Action="None" />
     <Rule Id="SA1013" Action="None" />

src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs

@@ -3,6 +3,7 @@
 using Ocelot.Configuration;
 using Ocelot.DependencyInjection;
 using Ocelot.Errors;
+using Ocelot.Errors.QoS;
 using Ocelot.Logging;
 using Ocelot.Provider.Polly.Interfaces;
 using Ocelot.Requester;

src/Ocelot.Provider.Polly/PollyQoSProvider.cs

@@ -13,6 +13,9 @@ public class PollyQoSProvider : IPollyQoSProvider<HttpResponseMessage>
     private readonly object _lockObject = new();
     private readonly IOcelotLogger _logger;
 
+    //todo: this should be configurable and available as global config parameter in ocelot.json
+    public const int DefaultRequestTimeoutSeconds = 90;
+
     private readonly HashSet<HttpStatusCode> _serverErrorCodes = new()
     {
         HttpStatusCode.InternalServerError,
@@ -63,14 +66,20 @@ private PollyPolicyWrapper<HttpResponseMessage> PollyPolicyWrapperFactory(Downst
                 .Or<TimeoutException>()
                 .CircuitBreakerAsync(route.QosOptions.ExceptionsAllowedBeforeBreaking,
                     durationOfBreak: TimeSpan.FromMilliseconds(route.QosOptions.DurationOfBreak),
-                    onBreak: (ex, breakDelay) => _logger.LogError(info + $"Breaking the circuit for {breakDelay.TotalMilliseconds} ms!", ex.Exception),
+                    onBreak: (ex, breakDelay) =>
+                        _logger.LogError(info + $"Breaking the circuit for {breakDelay.TotalMilliseconds} ms!",
+                            ex.Exception),
                     onReset: () => _logger.LogDebug(info + "Call OK! Closed the circuit again."),
                     onHalfOpen: () => _logger.LogDebug(info + "Half-open; Next call is a trial."));
         }
 
+        // No default set for polly timeout at the minute.
+        // Since a user could potentially set timeout value = 0, we need to handle this case.
+        // TODO throw an exception if the user sets timeout value = 0 or at least return a warning
+        // TODO the design in DelegatingHandlerHandlerFactory should be reviewed
         var timeoutPolicy = Policy
             .TimeoutAsync<HttpResponseMessage>(
-                TimeSpan.FromMilliseconds(route.QosOptions.TimeoutValue), 
+                TimeSpan.FromMilliseconds(route.QosOptions.TimeoutValue),
                 TimeoutStrategy.Pessimistic);
 
         return new PollyPolicyWrapper<HttpResponseMessage>(exceptionsAllowedBeforeBreakingPolicy, timeoutPolicy);

src/Ocelot/Configuration/Creator/HttpHandlerOptionsCreator.cs

@@ -8,6 +8,9 @@ public class HttpHandlerOptionsCreator : IHttpHandlerOptionsCreator
     {
         private readonly ITracer _tracer;
 
+        //todo: this should be configurable and available as global config parameter in ocelot.json
+        public const int DefaultPooledConnectionLifetimeSeconds = 120;
+
         public HttpHandlerOptionsCreator(IServiceProvider services)
         {
             _tracer = services.GetService<ITracer>();
@@ -19,9 +22,10 @@ public HttpHandlerOptions Create(FileHttpHandlerOptions options)
 
             //be sure that maxConnectionPerServer is in correct range of values
             var maxConnectionPerServer = (options.MaxConnectionsPerServer > 0) ? options.MaxConnectionsPerServer : int.MaxValue;
+            var pooledConnectionLifetime = TimeSpan.FromSeconds(options.PooledConnectionLifetimeSeconds ?? DefaultPooledConnectionLifetimeSeconds);
 
             return new HttpHandlerOptions(options.AllowAutoRedirect,
-                options.UseCookieContainer, useTracing, options.UseProxy, maxConnectionPerServer);
+                options.UseCookieContainer, useTracing, options.UseProxy, maxConnectionPerServer, pooledConnectionLifetime);
         }
     }
 }

src/Ocelot/Configuration/File/FileHttpHandlerOptions.cs

@@ -8,6 +8,7 @@ public FileHttpHandlerOptions()
             MaxConnectionsPerServer = int.MaxValue;
             UseCookieContainer = false;
             UseProxy = true;
+            PooledConnectionLifetimeSeconds = null;
         }
 
         public FileHttpHandlerOptions(FileHttpHandlerOptions from)
@@ -16,12 +17,14 @@ public FileHttpHandlerOptions(FileHttpHandlerOptions from)
             MaxConnectionsPerServer = from.MaxConnectionsPerServer;
             UseCookieContainer = from.UseCookieContainer;
             UseProxy = from.UseProxy;
+            PooledConnectionLifetimeSeconds = from.PooledConnectionLifetimeSeconds;
         }
 
         public bool AllowAutoRedirect { get; set; }
         public int MaxConnectionsPerServer { get; set; }
         public bool UseCookieContainer { get; set; }
         public bool UseProxy { get; set; }
-        public bool UseTracing { get; set; }
+        public bool UseTracing { get; set; }
+        public int? PooledConnectionLifetimeSeconds { get; set; }
     }
 }

src/Ocelot/Configuration/HttpHandlerOptions.cs

@@ -1,26 +1,27 @@
 namespace Ocelot.Configuration
 {
     /// <summary>
-    /// Describes configuration parameters for http handler,
-    /// that is created to handle a request to service.
+    /// Describes configuration parameters for http handler, that is created to handle a request to service.
     /// </summary>
     public class HttpHandlerOptions
     {
-        public HttpHandlerOptions(bool allowAutoRedirect, bool useCookieContainer, bool useTracing, bool useProxy, int maxConnectionsPerServer)
-        {
-            AllowAutoRedirect = allowAutoRedirect;
-            UseCookieContainer = useCookieContainer;
-            UseTracing = useTracing;
-            UseProxy = useProxy;
-            MaxConnectionsPerServer = maxConnectionsPerServer;
+        public HttpHandlerOptions(bool allowAutoRedirect, bool useCookieContainer, bool useTracing, bool useProxy,
+            int maxConnectionsPerServer, TimeSpan pooledConnectionLifeTime)
+        {
+            AllowAutoRedirect = allowAutoRedirect;
+            UseCookieContainer = useCookieContainer;
+            UseTracing = useTracing;
+            UseProxy = useProxy;
+            MaxConnectionsPerServer = maxConnectionsPerServer;
+            PooledConnectionLifeTime = pooledConnectionLifeTime;
         }
 
         /// <summary>
         /// Specify if auto redirect is enabled.
         /// </summary>
         /// <value>AllowAutoRedirect.</value>
-        public bool AllowAutoRedirect { get; }
-
+        public bool AllowAutoRedirect { get; }
+
         /// <summary>
         /// Specify is handler has to use a cookie container.
         /// </summary>
@@ -31,18 +32,24 @@ public HttpHandlerOptions(bool allowAutoRedirect, bool useCookieContainer, bool
         /// Specify is handler has to use a opentracing.
         /// </summary>
         /// <value>UseTracing.</value>
-        public bool UseTracing { get; }
-
+        public bool UseTracing { get; }
+
         /// <summary>
         /// Specify if handler has to use a proxy.
         /// </summary>
         /// <value>UseProxy.</value>
-        public bool UseProxy { get; }
-
+        public bool UseProxy { get; }
+
         /// <summary>
         /// Specify the maximum of concurrent connection to a network endpoint.
         /// </summary>
         /// <value>MaxConnectionsPerServer.</value>
         public int MaxConnectionsPerServer { get; }
+
+        /// <summary>
+        /// Specify the maximum of time a connection can be pooled.
+        /// </summary>
+        /// <value>PooledConnectionLifeTime.</value>
+        public TimeSpan PooledConnectionLifeTime { get; }
     }
-}
+}

src/Ocelot/Configuration/HttpHandlerOptionsBuilder.cs

@@ -1,4 +1,6 @@
-namespace Ocelot.Configuration
+using Ocelot.Configuration.Creator;
+
+namespace Ocelot.Configuration
 {
     public class HttpHandlerOptionsBuilder
     {
@@ -7,6 +9,7 @@ public class HttpHandlerOptionsBuilder
         private bool _useTracing;
         private bool _useProxy;
         private int _maxConnectionPerServer;
+        private TimeSpan _pooledConnectionLifetime = TimeSpan.FromSeconds(HttpHandlerOptionsCreator.DefaultPooledConnectionLifetimeSeconds);
 
         public HttpHandlerOptionsBuilder WithAllowAutoRedirect(bool input)
         {
@@ -38,9 +41,15 @@ public HttpHandlerOptionsBuilder WithUseMaxConnectionPerServer(int maxConnection
             return this;
         }
 
+        public HttpHandlerOptionsBuilder WithPooledConnectionLifetimeSeconds(TimeSpan pooledConnectionLifetime)
+        {
+            _pooledConnectionLifetime = pooledConnectionLifetime;
+            return this;
+        }
+
         public HttpHandlerOptions Build()
         {
-            return new HttpHandlerOptions(_allowAutoRedirect, _useCookieContainer, _useTracing, _useProxy, _maxConnectionPerServer);
+            return new HttpHandlerOptions(_allowAutoRedirect, _useCookieContainer, _useTracing, _useProxy, _maxConnectionPerServer, _pooledConnectionLifetime);
         }
     }
 }

src/Ocelot/DependencyInjection/OcelotBuilder.cs

@@ -107,18 +107,17 @@ public OcelotBuilder(IServiceCollection services, IConfiguration configurationRo
             Services.AddSingleton<IDownstreamRouteProvider, DownstreamRouteFinder.Finder.DownstreamRouteFinder>();
             Services.AddSingleton<IDownstreamRouteProvider, DownstreamRouteCreator>();
             Services.TryAddSingleton<IDownstreamRouteProviderFactory, DownstreamRouteProviderFactory>();
-            Services.TryAddSingleton<IHttpRequester, HttpClientHttpRequester>();
             Services.TryAddSingleton<IHttpResponder, HttpContextResponder>();
             Services.TryAddSingleton<IErrorsToHttpStatusCodeMapper, ErrorsToHttpStatusCodeMapper>();
             Services.TryAddSingleton<IRateLimitCounterHandler, MemoryCacheRateLimitCounterHandler>();
-            Services.TryAddSingleton<IHttpClientCache, MemoryHttpClientCache>();
             Services.TryAddSingleton<IRequestMapper, RequestMapper>();
             Services.TryAddSingleton<IHttpHandlerOptionsCreator, HttpHandlerOptionsCreator>();
             Services.TryAddSingleton<IDownstreamAddressesCreator, DownstreamAddressesCreator>();
             Services.TryAddSingleton<IDelegatingHandlerHandlerFactory, DelegatingHandlerHandlerFactory>();
             Services.TryAddSingleton<ICacheKeyGenerator, DefaultCacheKeyGenerator>();
             Services.TryAddSingleton<IOcelotConfigurationChangeTokenSource, OcelotConfigurationChangeTokenSource>();
             Services.TryAddSingleton<IOptionsMonitor<IInternalConfiguration>, OcelotConfigurationMonitor>();
+            Services.AddOcelotMessageInvokerPool();
 
             // See this for why we register this as singleton:
             // http://stackoverflow.com/questions/37371264/invalidoperationexception-unable-to-resolve-service-for-type-microsoft-aspnetc

src/Ocelot/Errors/QoS/RequestTimedOutError.cs

@@ -0,0 +1,11 @@
+using StatusCode = System.Net.HttpStatusCode;
+
+namespace Ocelot.Errors.QoS;
+
+public class RequestTimedOutError : Error
+{
+    public RequestTimedOutError(Exception exception)
+        : base($"Timeout making http request, exception: {exception}", OcelotErrorCode.RequestTimedOutError, (int)StatusCode.ServiceUnavailable)
+    {
+    }
+}

src/Ocelot/Middleware/DownstreamResponse.cs

@@ -1,21 +1,29 @@
 namespace Ocelot.Middleware
 {
-    public class DownstreamResponse
+    public class DownstreamResponse : IDisposable
     {
-        public DownstreamResponse(HttpContent content, HttpStatusCode statusCode, List<Header> headers, string reasonPhrase)
+        // To detect redundant calls
+        private bool _disposedValue;
+        private readonly HttpResponseMessage _responseMessage;
+
+        public DownstreamResponse(HttpContent content, HttpStatusCode statusCode, List<Header> headers,
+            string reasonPhrase)
         {
             Content = content;
             StatusCode = statusCode;
-            Headers = headers ?? new List<Header>();
+            Headers = headers ?? new();
             ReasonPhrase = reasonPhrase;
         }
 
         public DownstreamResponse(HttpResponseMessage response)
-            : this(response.Content, response.StatusCode, response.Headers.Select(x => new Header(x.Key, x.Value)).ToList(), response.ReasonPhrase)
+            : this(response.Content, response.StatusCode,
+                response.Headers.Select(x => new Header(x.Key, x.Value)).ToList(), response.ReasonPhrase)
         {
+            _responseMessage = response;
         }
 
-        public DownstreamResponse(HttpContent content, HttpStatusCode statusCode, IEnumerable<KeyValuePair<string, IEnumerable<string>>> headers, string reasonPhrase)
+        public DownstreamResponse(HttpContent content, HttpStatusCode statusCode,
+            IEnumerable<KeyValuePair<string, IEnumerable<string>>> headers, string reasonPhrase)
             : this(content, statusCode, headers.Select(x => new Header(x.Key, x.Value)).ToList(), reasonPhrase)
         {
         }
@@ -24,5 +32,31 @@ public DownstreamResponse(HttpContent content, HttpStatusCode statusCode, IEnume
         public HttpStatusCode StatusCode { get; }
         public List<Header> Headers { get; }
         public string ReasonPhrase { get; }
+
+        // Public implementation of Dispose pattern callable by consumers.
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
+
+        /// <summary>
+        /// We should make sure we dispose the content and response message to close the connection to the downstream service.
+        /// </summary>
+        protected virtual void Dispose(bool disposing)
+        {
+            if (_disposedValue)
+            {
+                return;
+            }
+
+            if (disposing)
+            {
+                Content?.Dispose();
+                _responseMessage?.Dispose();
+            }
+
+            _disposedValue = true;
+        }
     }
 }