From 51c0c53d2eb508d1b0ca1cb14ebd5a1812a56074 Mon Sep 17 00:00:00 2001 From: Adam Hammond Date: Tue, 16 Apr 2024 15:54:26 -0700 Subject: [PATCH 1/6] Fixes #5105 Add support for HttpRequestMessage objects containing StreamContent to the AddStandardHedgingHandler() resilience API. This change does not update any public API contracts. It updates internal and private API contracts only. Link to issue: https://github.com/dotnet/extensions/issues/5105 --- .../RequestMessageSnapshotStrategy.cs | 16 +- ...enceHttpClientBuilderExtensions.Hedging.cs | 50 +++- .../Internal/RequestMessageSnapshot.cs | 90 ++++++- .../Hedging/StandardHedgingTests.cs | 4 +- .../Resilience/RequestMessageSnapshotTests.cs | 245 +++++++++++++----- 5 files changed, 316 insertions(+), 89 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/Internals/RequestMessageSnapshotStrategy.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/Internals/RequestMessageSnapshotStrategy.cs index 8f484f1cbcd..5ca905f6ae1 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/Internals/RequestMessageSnapshotStrategy.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/Internals/RequestMessageSnapshotStrategy.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.IO; using System.Net.Http; using System.Threading.Tasks; using Microsoft.Shared.Diagnostics; @@ -25,10 +26,15 @@ internal sealed class RequestMessageSnapshotStrategy : ResilienceStrategy Throw.InvalidOperationException("The HTTP request message was not found in the resilience context."); } - using var snapshot = RequestMessageSnapshot.Create(request); - - context.Properties.Set(ResilienceKeys.RequestSnapshot, snapshot); - - return await callback(context, state).ConfigureAwait(context.ContinueOnCapturedContext); + try + { + using var snapshot = await RequestMessageSnapshot.CreateAsync(request).ConfigureAwait(context.ContinueOnCapturedContext); + context.Properties.Set(ResilienceKeys.RequestSnapshot, snapshot); + return await callback(context, state).ConfigureAwait(context.ContinueOnCapturedContext); + } + catch (IOException e) + { + return Outcome.FromException(e); + } } } diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs index a4e426c263f..01b391def06 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs @@ -3,7 +3,9 @@ using System; using System.Diagnostics.CodeAnalysis; +using System.IO; using System.Net.Http; +using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Http.Resilience; @@ -88,26 +90,48 @@ public static IStandardHedgingHandlerBuilder AddStandardHedgingHandler(this IHtt Throw.InvalidOperationException("Request message snapshot is not attached to the resilience context."); } - var requestMessage = snapshot.CreateRequestMessage(); - - // The secondary request message should use the action resilience context - requestMessage.SetResilienceContext(args.ActionContext); - - // replace the request message - args.ActionContext.Properties.Set(ResilienceKeys.RequestMessage, requestMessage); - + // if a routing strategy has been configured but it does not return the next route, then no more routes + // are availabe, stop hedging + Uri? route; if (args.PrimaryContext.Properties.TryGetValue(ResilienceKeys.RoutingStrategy, out var routingPipeline)) { - if (!routingPipeline.TryGetNextRoute(out var route)) + if (!routingPipeline.TryGetNextRoute(out route)) { - // no routes left, stop hedging return null; } - - requestMessage.RequestUri = requestMessage.RequestUri!.ReplaceHost(route); } + else + { + route = null; + } + + return async () => + { + Outcome? actionResult = null; + + try + { + var requestMessage = await snapshot.CreateRequestMessageAsync().ConfigureAwait(false); + + // The secondary request message should use the action resilience context + requestMessage.SetResilienceContext(args.ActionContext); + + // replace the request message + args.ActionContext.Properties.Set(ResilienceKeys.RequestMessage, requestMessage); + + if (route != null) + { + // replace the RequestUri of the request per the routing strategy + requestMessage.RequestUri = requestMessage.RequestUri!.ReplaceHost(route); + } + } + catch (IOException e) + { + actionResult = Outcome.FromException(e); + } - return () => args.Callback(args.ActionContext); + return actionResult ?? await args.Callback(args.ActionContext).ConfigureAwait(args.ActionContext.ContinueOnCapturedContext); + }; }; }); diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs index 2c2e55098c9..3ceb11e4f60 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs @@ -3,7 +3,9 @@ using System; using System.Collections.Generic; +using System.IO; using System.Net.Http; +using System.Threading.Tasks; using Microsoft.Extensions.ObjectPool; using Microsoft.Shared.Diagnostics; using Microsoft.Shared.Pools; @@ -22,21 +24,40 @@ internal sealed class RequestMessageSnapshot : IResettable, IDisposable private Version? _version; private HttpContent? _content; - public static RequestMessageSnapshot Create(HttpRequestMessage request) + [System.Diagnostics.CodeAnalysis.SuppressMessage("Resilience", "EA0014:The async method doesn't support cancellation", Justification = "Past the point of no cancellation.")] + public static async Task CreateAsync(HttpRequestMessage request) { + _ = Throw.IfNull(request); + var snapshot = _snapshots.Get(); - snapshot.Initialize(request); + await snapshot.InitializeAsync(request).ConfigureAwait(false); return snapshot; } - public HttpRequestMessage CreateRequestMessage() + [System.Diagnostics.CodeAnalysis.SuppressMessage("Resilience", "EA0014:The async method doesn't support cancellation", Justification = "Past the point of no cancellation.")] + public async Task CreateRequestMessageAsync() { + if (IsReset()) + { + throw new InvalidOperationException($"{nameof(CreateRequestMessageAsync)}() cannot be called on a snapshot object that has been reset and has not been initialized"); + } + var clone = new HttpRequestMessage(_method!, _requestUri) { - Content = _content, Version = _version! }; + if (_content is StreamContent) + { + (HttpContent? content, HttpContent? clonedContent) = await CloneContentAsync(_content).ConfigureAwait(false); + _content = content; + clone.Content = clonedContent; + } + else + { + clone.Content = _content; + } + #if NET5_0_OR_GREATER foreach (var prop in _properties) { @@ -56,6 +77,7 @@ public HttpRequestMessage CreateRequestMessage() return clone; } + [System.Diagnostics.CodeAnalysis.SuppressMessage("Critical Bug", "S2952:Classes should \"Dispose\" of members from the classes' own \"Dispose\" methods", Justification = "Handled by ObjectPool")] bool IResettable.TryReset() { _properties.Clear(); @@ -64,6 +86,13 @@ bool IResettable.TryReset() _method = null; _version = null; _requestUri = null; + if (_content is StreamContent) + { + // a snapshot's StreamContent is always a unique copy (deep clone) + // therefore, it is safe to dispose when snapshot is no longer needed + _content.Dispose(); + } + _content = null; return true; @@ -71,17 +100,62 @@ bool IResettable.TryReset() void IDisposable.Dispose() => _snapshots.Return(this); - private void Initialize(HttpRequestMessage request) + [System.Diagnostics.CodeAnalysis.SuppressMessage("Resilience", "EA0014:The async method doesn't support cancellation", Justification = "Past the point of no cancellation.")] + private static async Task<(HttpContent? content, HttpContent? clonedContent)> CloneContentAsync(HttpContent? content) { - if (request.Content is StreamContent) + HttpContent? clonedContent = null; + if (content != null) { - Throw.InvalidOperationException($"{nameof(StreamContent)} content cannot by cloned."); + HttpContent originalContent = content; + Stream originalRequestBody = await content.ReadAsStreamAsync().ConfigureAwait(false); + MemoryStream clonedRequestBody = new MemoryStream(); + await originalRequestBody.CopyToAsync(clonedRequestBody).ConfigureAwait(false); + clonedRequestBody.Position = 0; + if (originalRequestBody.CanSeek) + { + originalRequestBody.Position = 0; + } + else + { + originalRequestBody = new MemoryStream(); + await clonedRequestBody.CopyToAsync(originalRequestBody).ConfigureAwait(false); + originalRequestBody.Position = 0; + clonedRequestBody.Position = 0; + } + + clonedContent = new StreamContent(clonedRequestBody); + content = new StreamContent(originalRequestBody); + foreach (KeyValuePair> header in originalContent.Headers) + { + _ = clonedContent.Headers.TryAddWithoutValidation(header.Key, header.Value); + _ = content.Headers.TryAddWithoutValidation(header.Key, header.Value); + } } + return (content, clonedContent); + } + + private bool IsReset() + { + return _method == null; + } + + [System.Diagnostics.CodeAnalysis.SuppressMessage("Resilience", "EA0014:The async method doesn't support cancellation", Justification = "Past the point of no cancellation.")] + private async Task InitializeAsync(HttpRequestMessage request) + { _method = request.Method; _version = request.Version; _requestUri = request.RequestUri; - _content = request.Content; + if (request.Content is StreamContent) + { + (HttpContent? requestContent, HttpContent? clonedRequestContent) = await CloneContentAsync(request.Content).ConfigureAwait(false); + _content = clonedRequestContent; + request.Content = requestContent; + } + else + { + _content = request.Content; + } // headers _headers.AddRange(request.Headers); diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs index fb381f31e1b..327f43b19b8 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Hedging/StandardHedgingTests.cs @@ -103,7 +103,7 @@ public void Configure_ValidConfigurationSection_ShouldInitialize() } [Fact] - public void ActionGenerator_Ok() + public async Task ActionGenerator_Ok() { var options = Builder.Services.BuildServiceProvider().GetRequiredService>().Get(Builder.Name); var generator = options.Hedging.ActionGenerator; @@ -115,7 +115,7 @@ public void ActionGenerator_Ok() generator.Invoking(g => g(args)).Should().Throw().WithMessage("Request message snapshot is not attached to the resilience context."); using var request = new HttpRequestMessage(); - using var snapshot = RequestMessageSnapshot.Create(request); + using var snapshot = await RequestMessageSnapshot.CreateAsync(request).ConfigureAwait(false); primary.Properties.Set(ResilienceKeys.RequestSnapshot, snapshot); generator.Invoking(g => g(args)).Should().NotThrow(); } diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs index 4a5214f2a40..1fd310407b8 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs @@ -2,99 +2,222 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.IO; +using System.Linq; using System.Net.Http; using System.Text; +using System.Threading.Tasks; using FluentAssertions; using Microsoft.Extensions.Http.Resilience.Internal; using Xunit; namespace Microsoft.Extensions.Http.Resilience.Test.Resilience; -#pragma warning disable CS0618 // Type or member is obsolete -#pragma warning disable CS0618 // Type or member is obsolete - -public class RequestMessageSnapshotTests +public class RequestMessageSnapshotTests : IDisposable { + private readonly Uri _requestUri = new Uri("https://www.example.com/some-resource"); + private bool _disposedValue; + private HttpRequestMessage? _requestMessage; + + public RequestMessageSnapshotTests() + { + _requestMessage = new HttpRequestMessage(HttpMethod.Post, _requestUri); + } + [Fact] - public void CreateSnapshot_StreamContent_ShouldThrow() + public async Task CreateSnapshotAsync_RequestMessageContainsStringContent_Success() { - var initialRequest = new HttpRequestMessage - { - RequestUri = new Uri("https://dummy-uri.com?query=param"), - Method = HttpMethod.Get, - Content = new StreamContent(new MemoryStream()) - }; - - var exception = Assert.Throws(() => RequestMessageSnapshot.Create(initialRequest)); - Assert.Equal("StreamContent content cannot by cloned.", exception.Message); - initialRequest.Dispose(); + _requestMessage!.Content = new StringContent("some string content"); + AddRequestHeaders(_requestMessage); + AddRequestOptions(_requestMessage); + AddContentHeaders(_requestMessage!.Content); + using RequestMessageSnapshot snapshot = await RequestMessageSnapshot.CreateAsync(_requestMessage).ConfigureAwait(false); + using HttpRequestMessage clonedRequestMessage = await snapshot.CreateRequestMessageAsync().ConfigureAwait(false); + await AssertRequestMessagesAreEqual(_requestMessage, clonedRequestMessage).ConfigureAwait(false); } [Fact] - public void CreateSnapshot_CreatesClone() + public async Task CreateSnapshotAsync_RequestMessageContainsStreamContent_Success() { - using var request = CreateRequest(); - using var snapshot = RequestMessageSnapshot.Create(request); - var cloned = snapshot.CreateRequestMessage(); - AssertClonedMessage(request, cloned); + using var stream = new MemoryStream(); + using var streamWriter = new StreamWriter(stream); + await streamWriter.WriteAsync("some stream content").ConfigureAwait(false); + await streamWriter.FlushAsync().ConfigureAwait(false); + stream.Position = 0; + _requestMessage!.Content = new StreamContent(stream); + AddRequestHeaders(_requestMessage); + AddRequestOptions(_requestMessage); + AddContentHeaders(_requestMessage!.Content); + using RequestMessageSnapshot snapshot = await RequestMessageSnapshot.CreateAsync(_requestMessage).ConfigureAwait(false); + using HttpRequestMessage clonedRequestMessage = await snapshot.CreateRequestMessageAsync().ConfigureAwait(false); + await AssertRequestMessagesAreEqual(_requestMessage, clonedRequestMessage).ConfigureAwait(false); } [Fact] - public void CreateSnapshot_OriginalMessageChanged_SnapshotReturnsOriginalData() + public async Task CreateSnapshotAsync_RequestMessageContainsNonSeekableStreamContent_Success() { - using var request = CreateRequest(); - using var snapshot = RequestMessageSnapshot.Create(request); + using var stream = new NonSeekableStream("some stream content"); + _requestMessage!.Content = new StreamContent(stream); + AddRequestHeaders(_requestMessage); + AddRequestOptions(_requestMessage); + AddContentHeaders(_requestMessage!.Content); + using RequestMessageSnapshot snapshot = await RequestMessageSnapshot.CreateAsync(_requestMessage).ConfigureAwait(false); + using HttpRequestMessage clonedRequestMessage = await snapshot.CreateRequestMessageAsync().ConfigureAwait(false); + await AssertRequestMessagesAreEqual(_requestMessage, clonedRequestMessage).ConfigureAwait(false); + } - request.Properties["some-new-prop"] = "ABC"; - var cloned = snapshot.CreateRequestMessage(); - cloned.Properties.Should().NotContainKey("some-new-prop"); + [Fact] + public async Task CreateSnapshotAsync_RequestMessageHasNoContent_Success() + { + _requestMessage!.Method = HttpMethod.Get; + Assert.Null(_requestMessage!.Content); + AddRequestHeaders(_requestMessage); + AddRequestOptions(_requestMessage); + using RequestMessageSnapshot snapshot = await RequestMessageSnapshot.CreateAsync(_requestMessage).ConfigureAwait(false); + using HttpRequestMessage clonedRequestMessage = await snapshot.CreateRequestMessageAsync().ConfigureAwait(false); + await AssertRequestMessagesAreEqual(_requestMessage, clonedRequestMessage).ConfigureAwait(false); } - private static HttpRequestMessage CreateRequest() + [Fact] + public async Task CreateSnapshotAsync_RequestMessageIsNull_ThrowsException() { - var initialRequest = new HttpRequestMessage - { - RequestUri = new Uri("https://dummy-uri.com?query=param"), - Method = HttpMethod.Get, - Version = new Version(1, 1), - Content = new StringContent("{\"name\":\"John Doe\",\"age\":33}", Encoding.UTF8, "application/json") - }; - - initialRequest.Headers.Add("Authorization", "Bearer token"); - initialRequest.Properties.Add("A", "A"); - initialRequest.Properties.Add("B", "B"); - -#if NET8_0_OR_GREATER - // Whilst these API are marked as NET5_0_OR_GREATER we don't build .NET 5.0, - // and as such the API is available in .NET 8 onwards. - initialRequest.Options.Set(new HttpRequestOptionsKey("C"), "C"); - initialRequest.Options.Set(new HttpRequestOptionsKey("D"), "D"); + HttpRequestMessage? requestMessage = null; +#pragma warning disable CS8604 // Possible null reference argument. + _ = await Assert.ThrowsAsync(async () => await RequestMessageSnapshot.CreateAsync(requestMessage).ConfigureAwait(false)).ConfigureAwait(false); +#pragma warning restore CS8604 // Possible null reference argument. + } + + [Fact] + public async Task CreateSnapshotAsync_OriginalMessageChanged_SnapshotReturnsOriginalData() + { + using var snapshot = await RequestMessageSnapshot.CreateAsync(_requestMessage!).ConfigureAwait(false); + +#if NET5_0_OR_GREATER + _requestMessage!.Options.Set(new HttpRequestOptionsKey("Some.New.Request.Option"), "some new request option value"); +#else + _requestMessage!.Properties["Some.New.Request.Property"] = "some new request property value"; +#endif + + var cloned = await snapshot.CreateRequestMessageAsync().ConfigureAwait(false); + +#if NET5_0_OR_GREATER + cloned.Options.Should().NotContainKey("Some.New.Request.Option"); +#else + cloned.Properties.Should().NotContainKey("Some.New.Request.Property"); #endif - return initialRequest; } - private static void AssertClonedMessage(HttpRequestMessage initialRequest, HttpRequestMessage cloned) + public void Dispose() { - Assert.NotNull(cloned); - Assert.Equal(initialRequest.Method, cloned.Method); - Assert.Equal(initialRequest.RequestUri, cloned.RequestUri); - Assert.Equal(initialRequest.Content, cloned.Content); - Assert.Equal(initialRequest.Version, cloned.Version); + Dispose(true); + GC.SuppressFinalize(this); + } - Assert.NotNull(cloned.Headers.Authorization); + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + _requestMessage!.Dispose(); + _requestMessage = null; + } + + _disposedValue = true; + } + } - cloned.Properties["A"].Should().Be("A"); - cloned.Properties["B"].Should().Be("B"); + private static void AddContentHeaders(HttpContent content) + { + content.Headers.TryAddWithoutValidation("Some-Content-Header", "some content header value"); + content.Headers.TryAddWithoutValidation("Some-Other-Content-Header", "some other content header value"); + } -#if NET8_0_OR_GREATER - // Whilst these API are marked as NET5_0_OR_GREATER we don't build .NET 5.0, - // and as such the API is available in .NET 8 onwards. - initialRequest.Options.TryGetValue(new HttpRequestOptionsKey("C"), out var val).Should().BeTrue(); - val.Should().Be("C"); + private static void AddRequestHeaders(HttpRequestMessage requestMessage) + { + requestMessage.Headers.TryAddWithoutValidation("Some-Header", "some header value"); + requestMessage.Headers.TryAddWithoutValidation("Some-Other-Header", "some other header value"); + } - initialRequest.Options.TryGetValue(new HttpRequestOptionsKey("D"), out val).Should().BeTrue(); - val.Should().Be("D"); + private static void AddRequestOptions(HttpRequestMessage requestMessage) + { +#if NET5_0_OR_GREATER + requestMessage.Options.TryAdd("Some.Request.Option", "some request option value"); + requestMessage.Options.TryAdd("Some.Other.Request.Option", "some other request option value"); +#else + requestMessage.Properties["Some.Request.Property"] = "some request property value"; + requestMessage.Properties["Some.Other.Request.Property"] = "some other request property value"; #endif } + + private static async Task AssertRequestMessagesAreEqual(HttpRequestMessage requestMessageA, HttpRequestMessage requestMessageB) + { + Assert.NotNull(requestMessageA); + Assert.NotNull(requestMessageB); + Assert.NotSame(requestMessageA, requestMessageB); // assert no shallow copy + Assert.Equal(requestMessageA.Method, requestMessageB.Method); + Assert.Equal(requestMessageA.RequestUri?.AbsoluteUri, requestMessageB.RequestUri?.AbsoluteUri); + Assert.Equal(requestMessageA.Version, requestMessageB.Version); + if (requestMessageA.Content == null) + { + Assert.Null(requestMessageB.Content); + } + else if (requestMessageB.Content == null) + { + Assert.Null(requestMessageA.Content); + } + else + { + if (requestMessageA.Content is StreamContent) + { + Assert.NotSame(requestMessageA.Content, requestMessageB.Content); // assert no shallow copy + } + else + { + Assert.Same(requestMessageA.Content, requestMessageB.Content); // assert shallow copy + } + + Assert.Equal( + await requestMessageA.Content.ReadAsStringAsync().ConfigureAwait(false), + await requestMessageB.Content.ReadAsStringAsync().ConfigureAwait(false)); + + foreach (KeyValuePair> header in requestMessageA.Content.Headers) + { + Assert.Contains(requestMessageB.Content.Headers, (x) => x.Key == header.Key && x.Value.Any((y) => header.Value.Any((z) => z == y))); + } + } + + foreach (KeyValuePair> header in requestMessageA.Headers) + { + Assert.NotSame(requestMessageA.Headers, requestMessageB.Headers); // assert no shallow copy + Assert.Contains(requestMessageB.Headers, (x) => x.Key == header.Key + && x.Value.Any((y) => header.Value.Any((z) => z == y)) + && x.Value != header.Value); // assert no shallow copy + } + +#if NET5_0_OR_GREATER + foreach (KeyValuePair option in requestMessageA.Options) + { + Assert.NotSame(requestMessageA.Options, requestMessageB.Options); // assert no shallow copy + Assert.Contains(requestMessageB.Options, (x) => x.Key == option.Key && x.Value == option.Value); + } +#else + foreach (KeyValuePair property in requestMessageA.Properties) + { + Assert.NotSame(requestMessageA.Properties, requestMessageB.Properties); // assert no shallow copy + Assert.Contains(requestMessageB.Properties, (x) => x.Key == property.Key && x.Value == property.Value); + } +#endif + } + + private class NonSeekableStream : MemoryStream + { + public NonSeekableStream(string str) + : base(Encoding.UTF8.GetBytes(str), writable: false) + { + } + + public override bool CanSeek => false; + } } From 4fb26f9be9dfce1468988c11c085703c455f1155 Mon Sep 17 00:00:00 2001 From: Adam Hammond Date: Tue, 16 Apr 2024 16:57:03 -0700 Subject: [PATCH 2/6] Fixes #5105 Add support for HttpRequestMessage objects containing StreamContent to the AddStandardHedgingHandler() resilience API. This change does not update any public API contracts. It updates internal and private API contracts only. This is a small commit that is part of a larger PR. See the PR and its corresponding initial commit for the full set of changes. --- .../Resilience/RequestMessageSnapshotTests.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs index 1fd310407b8..970bc4755f0 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs @@ -10,6 +10,7 @@ using System.Threading.Tasks; using FluentAssertions; using Microsoft.Extensions.Http.Resilience.Internal; +using Microsoft.Extensions.ObjectPool; using Xunit; namespace Microsoft.Extensions.Http.Resilience.Test.Resilience; @@ -108,6 +109,18 @@ public async Task CreateSnapshotAsync_OriginalMessageChanged_SnapshotReturnsOrig #endif } + [Fact] + public async Task CreateRequestMessageAsync_SnapshotIsReset_ThrowsException() + { + _requestMessage!.Method = HttpMethod.Get; + Assert.Null(_requestMessage.Content); + AddRequestHeaders(_requestMessage); + AddRequestOptions(_requestMessage); + using RequestMessageSnapshot snapshot = await RequestMessageSnapshot.CreateAsync(_requestMessage).ConfigureAwait(false); + ((IResettable)snapshot).TryReset(); + _ = await Assert.ThrowsAsync(snapshot.CreateRequestMessageAsync); + } + public void Dispose() { Dispose(true); From 60accf3f3db66d5905c87291876b57fb56247771 Mon Sep 17 00:00:00 2001 From: Adam Hammond Date: Wed, 17 Apr 2024 11:54:51 -0700 Subject: [PATCH 3/6] Fixes #5105 Add support for HttpRequestMessage objects containing StreamContent to the AddStandardHedgingHandler() resilience API. This change does not update any public API contracts. It updates internal and private API contracts only. This is a small commit to update the ConfigureAwait arg on an async --- .../Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs index 01b391def06..095a246c1a8 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs @@ -111,7 +111,7 @@ public static IStandardHedgingHandlerBuilder AddStandardHedgingHandler(this IHtt try { - var requestMessage = await snapshot.CreateRequestMessageAsync().ConfigureAwait(false); + var requestMessage = await snapshot.CreateRequestMessageAsync().ConfigureAwait(args.ActionContext.ContinueOnCapturedContext); // The secondary request message should use the action resilience context requestMessage.SetResilienceContext(args.ActionContext); From 576fca58c1fefba902df649f4baa3e1a7342bc7e Mon Sep 17 00:00:00 2001 From: Adam Hammond Date: Thu, 18 Apr 2024 14:17:15 -0700 Subject: [PATCH 4/6] Fixes #5105 Add support for HttpRequestMessage objects containing StreamContent to the AddStandardHedgingHandler() resilience API. This change does not update any public API contracts. It updates internal and private API contracts only. This is a small commit to resolve comments made on the PR --- .../ResilienceHttpClientBuilderExtensions.Hedging.cs | 7 ++++--- .../Internal/RequestMessageSnapshot.cs | 10 +++++----- .../Internal/RequestMessageSnapshotStrategyTests.cs | 2 +- .../Resilience/RequestMessageSnapshotTests.cs | 9 +++++++-- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs index 095a246c1a8..1ed24cbef0a 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs @@ -90,11 +90,12 @@ public static IStandardHedgingHandlerBuilder AddStandardHedgingHandler(this IHtt Throw.InvalidOperationException("Request message snapshot is not attached to the resilience context."); } - // if a routing strategy has been configured but it does not return the next route, then no more routes - // are availabe, stop hedging + // if a routing strategy has been configured, get the next route from the routing strategy Uri? route; if (args.PrimaryContext.Properties.TryGetValue(ResilienceKeys.RoutingStrategy, out var routingPipeline)) { + // if a routing strategy has been configured but it does not return the next route, then no more routes + // are availabe, stop hedging if (!routingPipeline.TryGetNextRoute(out route)) { return null; @@ -121,7 +122,7 @@ public static IStandardHedgingHandlerBuilder AddStandardHedgingHandler(this IHtt if (route != null) { - // replace the RequestUri of the request per the routing strategy + // replace the Host on the RequestUri of the request per the routing strategy requestMessage.RequestUri = requestMessage.RequestUri!.ReplaceHost(route); } } diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs index 3ceb11e4f60..2c543f6ed2b 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs @@ -37,12 +37,12 @@ public static async Task CreateAsync(HttpRequestMessage [System.Diagnostics.CodeAnalysis.SuppressMessage("Resilience", "EA0014:The async method doesn't support cancellation", Justification = "Past the point of no cancellation.")] public async Task CreateRequestMessageAsync() { - if (IsReset()) + if (!IsInitialized()) { - throw new InvalidOperationException($"{nameof(CreateRequestMessageAsync)}() cannot be called on a snapshot object that has been reset and has not been initialized"); + throw new InvalidOperationException($"{nameof(CreateRequestMessageAsync)}() cannot be called on a snapshot object that has been reset and/or has not been initialized"); } - var clone = new HttpRequestMessage(_method!, _requestUri) + var clone = new HttpRequestMessage(_method!, _requestUri?.OriginalString) { Version = _version! }; @@ -135,9 +135,9 @@ private static async Task<(HttpContent? content, HttpContent? clonedContent)> Cl return (content, clonedContent); } - private bool IsReset() + private bool IsInitialized() { - return _method == null; + return _method != null; } [System.Diagnostics.CodeAnalysis.SuppressMessage("Resilience", "EA0014:The async method doesn't support cancellation", Justification = "Past the point of no cancellation.")] diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Internal/RequestMessageSnapshotStrategyTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Internal/RequestMessageSnapshotStrategyTests.cs index 06f21c7d9f1..cafa30e43a7 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Internal/RequestMessageSnapshotStrategyTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Internal/RequestMessageSnapshotStrategyTests.cs @@ -32,7 +32,7 @@ public async Task SendAsync_EnsureSnapshotAttached() } [Fact] - public void ExecuteAsync_requestMessageNotFound_Throws() + public void ExecuteAsync_RequestMessageNotFound_Throws() { var strategy = Create(); diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs index 970bc4755f0..3e57ad1df5b 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs @@ -112,10 +112,15 @@ public async Task CreateSnapshotAsync_OriginalMessageChanged_SnapshotReturnsOrig [Fact] public async Task CreateRequestMessageAsync_SnapshotIsReset_ThrowsException() { - _requestMessage!.Method = HttpMethod.Get; - Assert.Null(_requestMessage.Content); + using var stream = new MemoryStream(); + using var streamWriter = new StreamWriter(stream); + await streamWriter.WriteAsync("some stream content").ConfigureAwait(false); + await streamWriter.FlushAsync().ConfigureAwait(false); + stream.Position = 0; + _requestMessage!.Content = new StreamContent(stream); AddRequestHeaders(_requestMessage); AddRequestOptions(_requestMessage); + AddContentHeaders(_requestMessage!.Content); using RequestMessageSnapshot snapshot = await RequestMessageSnapshot.CreateAsync(_requestMessage).ConfigureAwait(false); ((IResettable)snapshot).TryReset(); _ = await Assert.ThrowsAsync(snapshot.CreateRequestMessageAsync); From f51e60e35c5c31ed6909545bc28e67d045b5b5ab Mon Sep 17 00:00:00 2001 From: Adam Hammond Date: Fri, 19 Apr 2024 11:08:29 -0700 Subject: [PATCH 5/6] Fixes #5105 Add support for HttpRequestMessage objects containing StreamContent to the AddStandardHedgingHandler() resilience API. This change does not update any public API contracts. It updates internal and private API contracts only. This is a small commit to resolve comments made on the PR. --- .../Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs | 2 +- .../Internal/RequestMessageSnapshot.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs index 1ed24cbef0a..4909f3ad451 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Hedging/ResilienceHttpClientBuilderExtensions.Hedging.cs @@ -120,7 +120,7 @@ public static IStandardHedgingHandlerBuilder AddStandardHedgingHandler(this IHtt // replace the request message args.ActionContext.Properties.Set(ResilienceKeys.RequestMessage, requestMessage); - if (route != null) + if (route is not null) { // replace the Host on the RequestUri of the request per the routing strategy requestMessage.RequestUri = requestMessage.RequestUri!.ReplaceHost(route); diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs index 2c543f6ed2b..b44b65b05b3 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs @@ -104,7 +104,7 @@ bool IResettable.TryReset() private static async Task<(HttpContent? content, HttpContent? clonedContent)> CloneContentAsync(HttpContent? content) { HttpContent? clonedContent = null; - if (content != null) + if (content is not null) { HttpContent originalContent = content; Stream originalRequestBody = await content.ReadAsStreamAsync().ConfigureAwait(false); @@ -137,7 +137,7 @@ private static async Task<(HttpContent? content, HttpContent? clonedContent)> Cl private bool IsInitialized() { - return _method != null; + return _method is not null; } [System.Diagnostics.CodeAnalysis.SuppressMessage("Resilience", "EA0014:The async method doesn't support cancellation", Justification = "Past the point of no cancellation.")] From deaf1cfbe588eb9b3285460a18feaf84461a32ee Mon Sep 17 00:00:00 2001 From: Adam Hammond Date: Mon, 22 Apr 2024 15:31:48 -0700 Subject: [PATCH 6/6] Fixes #5105 Add support for HttpRequestMessage objects containing StreamContent to the AddStandardHedgingHandler() resilience API. This change does not update any public API contracts. It updates internal and private API contracts only. This is a small commit to resolve comments made on the PR. --- .../Internal/RequestMessageSnapshot.cs | 6 ++-- .../RequestMessageSnapshotStrategyTests.cs | 36 +++++++++++++++++++ .../Resilience/RequestMessageSnapshotTests.cs | 2 +- 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs b/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs index b44b65b05b3..43f4d412ed5 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/RequestMessageSnapshot.cs @@ -25,7 +25,7 @@ internal sealed class RequestMessageSnapshot : IResettable, IDisposable private HttpContent? _content; [System.Diagnostics.CodeAnalysis.SuppressMessage("Resilience", "EA0014:The async method doesn't support cancellation", Justification = "Past the point of no cancellation.")] - public static async Task CreateAsync(HttpRequestMessage request) + public static async ValueTask CreateAsync(HttpRequestMessage request) { _ = Throw.IfNull(request); @@ -35,7 +35,7 @@ public static async Task CreateAsync(HttpRequestMessage } [System.Diagnostics.CodeAnalysis.SuppressMessage("Resilience", "EA0014:The async method doesn't support cancellation", Justification = "Past the point of no cancellation.")] - public async Task CreateRequestMessageAsync() + public async ValueTask CreateRequestMessageAsync() { if (!IsInitialized()) { @@ -101,7 +101,7 @@ bool IResettable.TryReset() void IDisposable.Dispose() => _snapshots.Return(this); [System.Diagnostics.CodeAnalysis.SuppressMessage("Resilience", "EA0014:The async method doesn't support cancellation", Justification = "Past the point of no cancellation.")] - private static async Task<(HttpContent? content, HttpContent? clonedContent)> CloneContentAsync(HttpContent? content) + private static async ValueTask<(HttpContent? content, HttpContent? clonedContent)> CloneContentAsync(HttpContent? content) { HttpContent? clonedContent = null; if (content is not null) diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Internal/RequestMessageSnapshotStrategyTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Internal/RequestMessageSnapshotStrategyTests.cs index cafa30e43a7..ff66487a4bc 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Internal/RequestMessageSnapshotStrategyTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Internal/RequestMessageSnapshotStrategyTests.cs @@ -2,7 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.IO; using System.Net.Http; +using System.Text; +using System.Threading; using System.Threading.Tasks; using FluentAssertions; using Microsoft.Extensions.Http.Resilience.Internal; @@ -39,5 +42,38 @@ public void ExecuteAsync_RequestMessageNotFound_Throws() strategy.Invoking(s => s.Execute(() => { })).Should().Throw(); } + [Fact] + public async Task ExecuteCoreAsync_IOExceptionThrownWhenCreatingSnapshot_ReturnsExceptionOutcome() + { + var strategy = Create(); + var context = ResilienceContextPool.Shared.Get(); + using var request = new HttpRequestMessage(HttpMethod.Post, new Uri("https://www.example.com/some-resource")); + using var stream = new StreamTestHelper("some stream content"); + request.Content = new StreamContent(stream); + context.Properties.Set(ResilienceKeys.RequestMessage, request); + + _ = await Assert.ThrowsAsync(async () => await strategy.ExecuteAsync(context => default, context)); + } + private static ResiliencePipeline Create() => new ResiliencePipelineBuilder().AddStrategy(_ => new RequestMessageSnapshotStrategy(), Mock.Of()).Build(); + + private class StreamTestHelper : MemoryStream + { + public StreamTestHelper(string str) + : base(Encoding.UTF8.GetBytes(str)) + { + } + + public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) => throw new IOException(); + +#if NET5_0_OR_GREATER + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) => throw new IOException(); + + public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) => throw new IOException(); +#else + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => throw new IOException(); + + public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => throw new IOException(); +#endif + } } diff --git a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs index 3e57ad1df5b..b9d7844fa0a 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Resilience.Tests/Resilience/RequestMessageSnapshotTests.cs @@ -123,7 +123,7 @@ public async Task CreateRequestMessageAsync_SnapshotIsReset_ThrowsException() AddContentHeaders(_requestMessage!.Content); using RequestMessageSnapshot snapshot = await RequestMessageSnapshot.CreateAsync(_requestMessage).ConfigureAwait(false); ((IResettable)snapshot).TryReset(); - _ = await Assert.ThrowsAsync(snapshot.CreateRequestMessageAsync); + _ = await Assert.ThrowsAsync(async () => await snapshot.CreateRequestMessageAsync().ConfigureAwait(false)); } public void Dispose()