From 4d71be5bb7d99f0097ccf60b974f8913fa12492c Mon Sep 17 00:00:00 2001 From: justcoding121 Date: Tue, 17 Aug 2021 17:23:57 -0600 Subject: [PATCH] #826 Fix retry logic --- ... => RetryableServerConnectionException.cs} | 9 +++---- src/Titanium.Web.Proxy/Network/RetryPolicy.cs | 8 +++--- .../Network/Streams/HttpServerStream.cs | 26 +++++++------------ .../Network/Streams/HttpStream.cs | 17 ++++++++++-- src/Titanium.Web.Proxy/ProxyServer.cs | 2 +- src/Titanium.Web.Proxy/RequestHandler.cs | 9 +++++-- .../NestedProxyTests.cs | 4 +-- 7 files changed, 41 insertions(+), 34 deletions(-) rename src/Titanium.Web.Proxy/Exceptions/{ServerConnectionException.cs => RetryableServerConnectionException.cs} (51%) diff --git a/src/Titanium.Web.Proxy/Exceptions/ServerConnectionException.cs b/src/Titanium.Web.Proxy/Exceptions/RetryableServerConnectionException.cs similarity index 51% rename from src/Titanium.Web.Proxy/Exceptions/ServerConnectionException.cs rename to src/Titanium.Web.Proxy/Exceptions/RetryableServerConnectionException.cs index d885c74e7..ad07df990 100644 --- a/src/Titanium.Web.Proxy/Exceptions/ServerConnectionException.cs +++ b/src/Titanium.Web.Proxy/Exceptions/RetryableServerConnectionException.cs @@ -3,12 +3,12 @@ namespace Titanium.Web.Proxy.Exceptions { /// - /// The server connection was closed upon first read with the new connection from pool. + /// The server connection was closed upon first write with the new connection from pool. /// Should retry the request with a new connection. /// - public class ServerConnectionException : ProxyException + public class RetryableServerConnectionException : ProxyException { - internal ServerConnectionException(string message) : base(message) + internal RetryableServerConnectionException(string message) : base(message) { } @@ -17,9 +17,8 @@ internal ServerConnectionException(string message) : base(message) /// /// /// - internal ServerConnectionException(string message, Exception e) : base(message, e) + internal RetryableServerConnectionException(string message, Exception e) : base(message, e) { } - } } diff --git a/src/Titanium.Web.Proxy/Network/RetryPolicy.cs b/src/Titanium.Web.Proxy/Network/RetryPolicy.cs index a2f724554..19863cba0 100644 --- a/src/Titanium.Web.Proxy/Network/RetryPolicy.cs +++ b/src/Titanium.Web.Proxy/Network/RetryPolicy.cs @@ -35,14 +35,12 @@ internal async Task ExecuteAsync(Func ReadResponseStatus(CancellationToken cancellationToken = default) { - try - { - string httpStatus = await ReadLineAsync(cancellationToken) ?? - throw new ServerConnectionException("Server connection was closed."); - - if (httpStatus == string.Empty) - { - // is this really possible? - httpStatus = await ReadLineAsync(cancellationToken) ?? - throw new ServerConnectionException("Server connection was closed. Response status is empty."); - } + string httpStatus = await ReadLineAsync(cancellationToken) ?? + throw new IOException("Invalid http status code."); - Response.ParseResponseLine(httpStatus, out var version, out int statusCode, out string description); - return new ResponseStatusInfo { Version = version, StatusCode = statusCode, Description = description }; - } - catch (Exception e) when (!(e is ServerConnectionException)) + if (httpStatus == string.Empty) { - throw new ServerConnectionException("Server connection was closed. Exception while reading the response status.", e); + // is this really possible? + httpStatus = await ReadLineAsync(cancellationToken) ?? + throw new IOException("Response status is empty."); } + + Response.ParseResponseLine(httpStatus, out var version, out int statusCode, out string description); + return new ResponseStatusInfo { Version = version, StatusCode = statusCode, Description = description }; + } } } diff --git a/src/Titanium.Web.Proxy/Network/Streams/HttpStream.cs b/src/Titanium.Web.Proxy/Network/Streams/HttpStream.cs index 3f4e2f75b..9e38996ee 100644 --- a/src/Titanium.Web.Proxy/Network/Streams/HttpStream.cs +++ b/src/Titanium.Web.Proxy/Network/Streams/HttpStream.cs @@ -932,7 +932,21 @@ public ValueTask WriteLineAsync(string value, CancellationToken cancellationToke internal async Task WriteHeadersAsync(HeaderBuilder headerBuilder, CancellationToken cancellationToken = default) { var buffer = headerBuilder.GetBuffer(); - await WriteAsync(buffer.Array, buffer.Offset, buffer.Count, true, cancellationToken); + + try + { + await WriteAsync(buffer.Array, buffer.Offset, buffer.Count, true, cancellationToken); + } + catch (IOException e) + { + //throw this as ServerConnectionException so that RetryPolicy can retry with a new server connection. + if (this is HttpServerStream) + { + throw new RetryableServerConnectionException("Server connection was closed. Exception while sending request line and headers.", e); + } + + throw; + } } /// @@ -1226,7 +1240,6 @@ private async Task copyBytesToStream(IHttpStreamWriter writer, long count, bool protected async ValueTask WriteAsync(RequestResponseBase requestResponse, HeaderBuilder headerBuilder, CancellationToken cancellationToken = default) { var body = requestResponse.CompressBodyAndUpdateContentLength(); - headerBuilder.WriteHeaders(requestResponse.Headers); await WriteHeadersAsync(headerBuilder, cancellationToken); diff --git a/src/Titanium.Web.Proxy/ProxyServer.cs b/src/Titanium.Web.Proxy/ProxyServer.cs index 938691a04..a0bca42c0 100644 --- a/src/Titanium.Web.Proxy/ProxyServer.cs +++ b/src/Titanium.Web.Proxy/ProxyServer.cs @@ -123,7 +123,7 @@ public ProxyServer(string? rootCertificateName, string? rootCertificateIssuerNam /// /// Number of times to retry upon network failures when connection pool is enabled. /// - public int NetworkFailureRetryAttempts { get; set; } = 0; + public int NetworkFailureRetryAttempts { get; set; } = 1; /// /// Is the proxy currently running? diff --git a/src/Titanium.Web.Proxy/RequestHandler.cs b/src/Titanium.Web.Proxy/RequestHandler.cs index 103f309f9..005c2daa7 100644 --- a/src/Titanium.Web.Proxy/RequestHandler.cs +++ b/src/Titanium.Web.Proxy/RequestHandler.cs @@ -288,8 +288,13 @@ private async Task handleHttpSessionRequest(SessionEventArgs args, noCache, cancellationToken); - // for connection pool, retry fails until cache is exhausted. - return await retryPolicy().ExecuteAsync(async connection => + /// Retry with new connection if the initial stream.WriteAsync call to server fails. + /// i.e if request line and headers failed to get send. + /// Do not retry after reading data from client stream, + /// because subsequent try will not have data to read from client + /// and will hang at clientStream.ReadAsync call. + /// So, throw RetryableServerConnectionException only when we are sure we can retry safely. + return await retryPolicy().ExecuteAsync(async connection => { // set the connection and send request headers args.HttpClient.SetConnection(connection); diff --git a/tests/Titanium.Web.Proxy.IntegrationTests/NestedProxyTests.cs b/tests/Titanium.Web.Proxy.IntegrationTests/NestedProxyTests.cs index 49d7aa3f9..5a95aa8fd 100644 --- a/tests/Titanium.Web.Proxy.IntegrationTests/NestedProxyTests.cs +++ b/tests/Titanium.Web.Proxy.IntegrationTests/NestedProxyTests.cs @@ -195,14 +195,13 @@ public async Task Nested_Proxy_Farm_With_Connection_Cache_Should_Not_Hang() var proxies2 = new List(); //create a level 2 upstream proxy farm that forwards to server - for (int i = 0; i < 2; i++) + for (int i = 0; i < 10; i++) { var proxy2 = testSuite.GetProxy(); proxy2.ProxyBasicAuthenticateFunc += (_, _, _) => { return Task.FromResult(true); }; - proxy2.MaxCachedConnections = 100; proxies2.Add(proxy2); } @@ -213,7 +212,6 @@ public async Task Nested_Proxy_Farm_With_Connection_Cache_Should_Not_Hang() { var proxy1 = testSuite.GetProxy(); var proxy2 = proxies2[rnd.Next() % proxies2.Count]; - proxy1.MaxCachedConnections = 100; var explicitEndpoint = proxy1.ProxyEndPoints.OfType().First(); explicitEndpoint.BeforeTunnelConnectRequest += (_, e) => {