Fix HttpTransformer-related issues

docs/docfx/articles/direct-forwarding.md

@@ -80,10 +80,10 @@ public void Configure(IApplicationBuilder app, IHttpForwarder forwarder)
 private class CustomTransformer : HttpTransformer
 {
     public override async ValueTask TransformRequestAsync(HttpContext httpContext,
-        HttpRequestMessage proxyRequest, string destinationPrefix)
+        HttpRequestMessage proxyRequest, string destinationPrefix, CancellationToken cancellationToken)
     {
         // Copy all request headers
-        await base.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix);
+        await base.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, cancellationToken);
 
         // Customize the query string:
         var queryContext = new QueryTransformContext(httpContext.Request);

samples/ReverseProxy.Direct.Sample/Startup.cs

@@ -5,6 +5,7 @@
 using System.Diagnostics;
 using System.Net;
 using System.Net.Http;
+using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Http;
@@ -104,10 +105,10 @@ private class CustomTransformer : HttpTransformer
             /// <param name="proxyRequest">The outgoing proxy request.</param>
             /// <param name="destinationPrefix">The uri prefix for the selected destination server which can be used to create
             /// the RequestUri.</param>
-            public override async ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix)
+            public override async ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix, CancellationToken cancellationToken)
             {
                 // Copy all request headers
-                await base.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix);
+                await base.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, cancellationToken);
 
                 // Customize the query string:
                 var queryContext = new QueryTransformContext(httpContext.Request);

src/ReverseProxy/Forwarder/ForwarderError.cs

@@ -98,4 +98,9 @@ public enum ForwarderError : int
     /// The configured destinations may have been excluded due to heath or other considerations.
     /// </summary>
     NoAvailableDestinations,
+
+    /// <summary>
+    /// Failed while creating the request message.
+    /// </summary>
+    RequestCreation,
 }

src/ReverseProxy/Forwarder/HttpForwarder.cs

@@ -117,6 +117,12 @@ public async ValueTask<ForwarderError> SendAsync(
             throw new ArgumentException($"The http client must be of type HttpMessageInvoker, not HttpClient", nameof(httpClient));
         }
 
+        // "http://a".Length = 8
+        if (destinationPrefix is null || destinationPrefix.Length < 8)
+        {
+            throw new ArgumentException("Invalid destination prefix.", nameof(destinationPrefix));
+        }
+
         ForwarderTelemetry.Log.ForwarderStart(destinationPrefix);
 
         var activityCancellationSource = ActivityCancellationTokenSource.Rent(requestConfig?.ActivityTimeout ?? DefaultTimeout, context.RequestAborted, cancellationToken);
@@ -128,23 +134,26 @@ public async ValueTask<ForwarderError> SendAsync(
             // See https://github.com/microsoft/reverse-proxy/issues/118 for design discussion.
             var isStreamingRequest = isClientHttp2OrGreater && ProtocolHelper.IsGrpcContentType(context.Request.ContentType);
 
-            // :: Step 1-3: Create outgoing HttpRequestMessage
-            var (destinationRequest, requestContent, tryDowngradingH2WsOnFailure) = await CreateRequestMessageAsync(
-                context, destinationPrefix, transformer, requestConfig, isStreamingRequest, activityCancellationSource);
-
-            // Transforms generated a response, do not proxy.
-            if (RequestUtilities.IsResponseSet(context.Response))
-            {
-                Log.NotProxying(_logger, context.Response.StatusCode);
-                return ForwarderError.None;
-            }
-
-            Log.Proxying(_logger, destinationRequest, isStreamingRequest);
-
-            // :: Step 4: Send the outgoing request using HttpClient
+            HttpRequestMessage? destinationRequest = null;
+            StreamCopyHttpContent? requestContent = null;
             HttpResponseMessage destinationResponse;
             try
             {
+                // :: Step 1-3: Create outgoing HttpRequestMessage
+                bool tryDowngradingH2WsOnFailure;
+                (destinationRequest, requestContent, tryDowngradingH2WsOnFailure) = await CreateRequestMessageAsync(
+                    context, destinationPrefix, transformer, requestConfig, isStreamingRequest, activityCancellationSource);
+
+                // Transforms generated a response, do not proxy.
+                if (RequestUtilities.IsResponseSet(context.Response))
+                {
+                    Log.NotProxying(_logger, context.Response.StatusCode);
+                    return ForwarderError.None;
+                }
+
+                Log.Proxying(_logger, destinationRequest, isStreamingRequest);
+
+                // :: Step 4: Send the outgoing request using HttpClient
                 ForwarderTelemetry.Log.ForwarderStage(ForwarderStage.SendAsyncStart);
 
                 try
@@ -190,7 +199,8 @@ public async ValueTask<ForwarderError> SendAsync(
             }
             catch (Exception requestException)
             {
-                return await HandleRequestFailureAsync(context, requestContent, requestException, transformer, activityCancellationSource);
+                return await HandleRequestFailureAsync(context, requestContent, requestException, transformer, activityCancellationSource,
+                    failedDuringRequestCreation: destinationRequest is null);
             }
 
             ForwarderTelemetry.Log.ForwarderStage(ForwarderStage.SendAsyncStop);
@@ -320,12 +330,6 @@ public async ValueTask<ForwarderError> SendAsync(
     private async ValueTask<(HttpRequestMessage, StreamCopyHttpContent?, bool)> CreateRequestMessageAsync(HttpContext context, string destinationPrefix,
         HttpTransformer transformer, ForwarderRequestConfig? requestConfig, bool isStreamingRequest, ActivityCancellationTokenSource activityToken)
     {
-        // "http://a".Length = 8
-        if (destinationPrefix is null || destinationPrefix.Length < 8)
-        {
-            throw new ArgumentException("Invalid destination prefix.", nameof(destinationPrefix));
-        }
-
         var destinationRequest = new HttpRequestMessage();
 
         var upgradeFeature = context.Features.Get<IHttpUpgradeFeature>();
@@ -618,12 +622,14 @@ private ForwarderError HandleRequestBodyFailure(HttpContext context, StreamCopyR
         return requestBodyError;
     }
 
-    private async ValueTask<ForwarderError> HandleRequestFailureAsync(HttpContext context, StreamCopyHttpContent? requestContent, Exception requestException, HttpTransformer transformer, CancellationTokenSource requestCancellationSource)
+    private async ValueTask<ForwarderError> HandleRequestFailureAsync(HttpContext context, StreamCopyHttpContent? requestContent, Exception requestException,
+        HttpTransformer transformer, ActivityCancellationTokenSource requestCancellationSource, bool failedDuringRequestCreation)
     {
         if (requestException is OperationCanceledException)
         {
-            if (context.RequestAborted.IsCancellationRequested)
+            if (requestCancellationSource.CancelledByLinkedToken)
             {
+                // Either the client went away (HttpContext.RequestAborted) or the CancellationToken provided to SendAsync was signaled.
                 return await ReportErrorAsync(ForwarderError.RequestCanceled, StatusCodes.Status502BadGateway);
             }
             else
@@ -641,13 +647,13 @@ private async ValueTask<ForwarderError> HandleRequestFailureAsync(HttpContext co
             if (requestBodyCopyResult != StreamCopyResult.Success)
             {
                 var error = HandleRequestBodyFailure(context, requestBodyCopyResult, requestBodyException!, requestException);
-                await transformer.TransformResponseAsync(context, proxyResponse: null);
+                await transformer.TransformResponseAsync(context, proxyResponse: null, requestCancellationSource.Token);
                 return error;
             }
         }
 
         // We couldn't communicate with the destination.
-        return await ReportErrorAsync(ForwarderError.Request, StatusCodes.Status502BadGateway);
+        return await ReportErrorAsync(failedDuringRequestCreation ? ForwarderError.RequestCreation : ForwarderError.Request, StatusCodes.Status502BadGateway);
 
         async ValueTask<ForwarderError> ReportErrorAsync(ForwarderError error, int statusCode)
         {
@@ -660,7 +666,7 @@ async ValueTask<ForwarderError> ReportErrorAsync(ForwarderError error, int statu
                 await requestContent.ConsumptionTask;
             }
 
-            await transformer.TransformResponseAsync(context, null);
+            await transformer.TransformResponseAsync(context, null, requestCancellationSource.Token);
             return error;
         }
     }
@@ -1051,6 +1057,7 @@ private static string GetMessage(ForwarderError error)
             {
                 ForwarderError.None => throw new NotSupportedException("A more specific error must be used"),
                 ForwarderError.Request => "An error was encountered before receiving a response.",
+                ForwarderError.RequestCreation => "An error was encountered while creating the request message.",
                 ForwarderError.RequestTimedOut => "The request timed out before receiving a response.",
                 ForwarderError.RequestCanceled => "The request was canceled before receiving a response.",
                 ForwarderError.RequestBodyCanceled => "Copying the request body was canceled.",

src/ReverseProxy/Forwarder/HttpTransformer.cs

@@ -69,7 +69,9 @@ private static bool IsBodylessStatusCode(HttpStatusCode statusCode) =>
     /// <param name="destinationPrefix">The uri prefix for the selected destination server which can be used to create the RequestUri.</param>
     /// <param name="cancellationToken">Indicates that the request is being canceled.</param>
     public virtual ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix, CancellationToken cancellationToken)
+#pragma warning disable CS0618 // We're calling the overload without the CancellationToken for backwards compatibility.
         => TransformRequestAsync(httpContext, proxyRequest, destinationPrefix);
+#pragma warning restore CS0618
 
     /// <summary>
     /// A callback that is invoked prior to sending the proxied request. All HttpRequestMessage fields are
@@ -84,6 +86,7 @@ public virtual ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequ
     /// <param name="httpContext">The incoming request.</param>
     /// <param name="proxyRequest">The outgoing proxy request.</param>
     /// <param name="destinationPrefix">The uri prefix for the selected destination server which can be used to create the RequestUri.</param>
+    [Obsolete("This overload of TransformRequestAsync is obsolete. Override and use the overload accepting a CancellationToken instead.")]
     public virtual ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix)
     {
         foreach (var header in httpContext.Request.Headers)
@@ -149,7 +152,9 @@ public virtual ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequ
     /// that returns false may send an alternate response inline or return control to the caller for it to retry, respond, 
     /// etc.</returns>
     public virtual ValueTask<bool> TransformResponseAsync(HttpContext httpContext, HttpResponseMessage? proxyResponse, CancellationToken cancellationToken)
+#pragma warning disable CS0618 // We're calling the overload without the CancellationToken for backwards compatibility.
         => TransformResponseAsync(httpContext, proxyResponse);
+#pragma warning restore CS0618
 
     /// <summary>
     /// A callback that is invoked when the proxied response is received. The status code and reason phrase will be copied
@@ -162,6 +167,7 @@ public virtual ValueTask<bool> TransformResponseAsync(HttpContext httpContext, H
     /// <returns>A bool indicating if the response should be proxied to the client or not. A derived implementation
     /// that returns false may send an alternate response inline or return control to the caller for it to retry, respond,
     /// etc.</returns>
+    [Obsolete("This overload of TransformResponseAsync is obsolete. Override and use the overload accepting a CancellationToken instead.")]
     public virtual ValueTask<bool> TransformResponseAsync(HttpContext httpContext, HttpResponseMessage? proxyResponse)
     {
         if (proxyResponse is null)
@@ -212,14 +218,17 @@ public virtual ValueTask<bool> TransformResponseAsync(HttpContext httpContext, H
     /// <param name="proxyResponse">The response from the destination.</param>
     /// <param name="cancellationToken">Indicates that the request is being canceled.</param>
     public virtual ValueTask TransformResponseTrailersAsync(HttpContext httpContext, HttpResponseMessage proxyResponse, CancellationToken cancellationToken)
+#pragma warning disable CS0618 // We're calling the overload without the CancellationToken for backwards compatibility.
         => TransformResponseTrailersAsync(httpContext, proxyResponse);
+#pragma warning restore CS0618
 
     /// <summary>
     /// A callback that is invoked after the response body to modify trailers, if supported. The trailers will be
     /// copied to the HttpContext.Response by the base implementation.
     /// </summary>
     /// <param name="httpContext">The incoming request.</param>
     /// <param name="proxyResponse">The response from the destination.</param>
+    [Obsolete("This overload of TransformResponseTrailersAsync is obsolete. Override and use the overload accepting a CancellationToken instead.")]
     public virtual ValueTask TransformResponseTrailersAsync(HttpContext httpContext, HttpResponseMessage proxyResponse)
     {
         // NOTE: Deliberately not using `context.Response.SupportsTrailers()`, `context.Response.AppendTrailer(...)`

src/ReverseProxy/Forwarder/RequestTransformer.cs

@@ -3,12 +3,13 @@
 
 using System;
 using System.Net.Http;
+using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
 
 namespace Yarp.ReverseProxy.Forwarder;
 
-internal class RequestTransformer : HttpTransformer
+internal sealed class RequestTransformer : HttpTransformer
 {
     private readonly Func<HttpContext, HttpRequestMessage, ValueTask> _requestTransform;
 
@@ -17,9 +18,9 @@ public RequestTransformer(Func<HttpContext, HttpRequestMessage, ValueTask> reque
         _requestTransform = requestTransform;
     }
 
-    public override async ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix)
+    public override async ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix, CancellationToken cancellationToken)
     {
-        await base.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix);
+        await base.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, cancellationToken);
         await _requestTransform(httpContext, proxyRequest);
     }
 }

src/ReverseProxy/Transforms/Builder/StructuredTransformer.cs

@@ -64,11 +64,29 @@ internal StructuredTransformer(bool? copyRequestHeaders, bool? copyResponseHeade
     /// </summary>
     internal ResponseTrailersTransform[] ResponseTrailerTransforms { get; }
 
+#pragma warning disable CS0672 // We're overriding the obsolete overloads to preserve backwards compatibility.
+    public override ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix) =>
+        TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, CancellationToken.None);
+
+    public override ValueTask<bool> TransformResponseAsync(HttpContext httpContext, HttpResponseMessage? proxyResponse) =>
+        TransformResponseAsync(httpContext, proxyResponse, CancellationToken.None);
+
+    public override ValueTask TransformResponseTrailersAsync(HttpContext httpContext, HttpResponseMessage proxyResponse) =>
+        TransformResponseTrailersAsync(httpContext, proxyResponse, CancellationToken.None);
+#pragma warning restore
+
     public override async ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix, CancellationToken cancellationToken)
     {
         if (ShouldCopyRequestHeaders.GetValueOrDefault(true))
         {
-            await base.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, cancellationToken);
+            // We own the base implementation and know it doesn't make use of the cancellation token.
+            // We're intentionally calling the overload without it to avoid it calling back into this derived implementation, causing a stack overflow.
+
+#pragma warning disable CA2016 // Forward the 'CancellationToken' parameter to methods
+#pragma warning disable CS0618 // Type or member is obsolete
+            await base.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix);
+#pragma warning restore CS0618
+#pragma warning restore CA2016
         }
 
         if (RequestTransforms.Length == 0)
@@ -111,7 +129,14 @@ public override async ValueTask<bool> TransformResponseAsync(HttpContext httpCon
     {
         if (ShouldCopyResponseHeaders.GetValueOrDefault(true))
         {
-            await base.TransformResponseAsync(httpContext, proxyResponse, cancellationToken);
+            // We own the base implementation and know it doesn't make use of the cancellation token.
+            // We're intentionally calling the overload without it to avoid it calling back into this derived implementation, causing a stack overflow.
+
+#pragma warning disable CA2016 // Forward the 'CancellationToken' parameter to methods
+#pragma warning disable CS0618 // Type or member is obsolete
+            await base.TransformResponseAsync(httpContext, proxyResponse);
+#pragma warning restore CS0618
+#pragma warning restore CA2016
         }
 
         if (ResponseTransforms.Length == 0)
@@ -139,7 +164,14 @@ public override async ValueTask TransformResponseTrailersAsync(HttpContext httpC
     {
         if (ShouldCopyResponseTrailers.GetValueOrDefault(true))
         {
-            await base.TransformResponseTrailersAsync(httpContext, proxyResponse, cancellationToken);
+            // We own the base implementation and know it doesn't make use of the cancellation token.
+            // We're intentionally calling the overload without it to avoid it calling back into this derived implementation, causing a stack overflow.
+
+#pragma warning disable CA2016 // Forward the 'CancellationToken' parameter to methods
+#pragma warning disable CS0618 // Type or member is obsolete
+            await base.TransformResponseTrailersAsync(httpContext, proxyResponse);
+#pragma warning restore CS0618
+#pragma warning restore CA2016
         }
 
         if (ResponseTrailerTransforms.Length == 0)