From 8b81e4da156512702c5131098a14387279efcb40 Mon Sep 17 00:00:00 2001 From: Joshua Harms Date: Thu, 16 May 2024 10:58:28 -0500 Subject: [PATCH 1/2] fix: Dispose cloned request messages in ExponentialRetryPolicy --- .../ExponentialRetry/ExponentialRetryPolicyTests.cs | 13 +++++++++++++ .../ExponentialRetry/ExponentialRetryPolicy.cs | 6 +++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/ShopifySharp.Tests/Infrastructure/Policies/ExponentialRetry/ExponentialRetryPolicyTests.cs b/ShopifySharp.Tests/Infrastructure/Policies/ExponentialRetry/ExponentialRetryPolicyTests.cs index 7d4a0e9e9..b77e8ede6 100644 --- a/ShopifySharp.Tests/Infrastructure/Policies/ExponentialRetry/ExponentialRetryPolicyTests.cs +++ b/ShopifySharp.Tests/Infrastructure/Policies/ExponentialRetry/ExponentialRetryPolicyTests.cs @@ -80,6 +80,19 @@ public async Task Run_ShouldReturnResult() result.Result.Should().Be(expectedValue); } + [Fact] + public async Task Run_ShouldDisposeClonedRequestMessages() + { + _cloneableRequestMessage.When(x => x.Dispose()) + .Throw(); + + var policy = SetupPolicy(); + var act = () => policy.Run(_cloneableRequestMessage, _executeRequest, CancellationToken.None); + + await act.Should().ThrowAsync(); + _cloneableRequestMessage.Received(1).Dispose(); + } + [Fact] public async Task Run_ShouldThrowWhenRequestIsNotRetriableAsync() { diff --git a/ShopifySharp/Infrastructure/Policies/ExponentialRetry/ExponentialRetryPolicy.cs b/ShopifySharp/Infrastructure/Policies/ExponentialRetry/ExponentialRetryPolicy.cs index 9b913b1f7..9fbf09dc1 100644 --- a/ShopifySharp/Infrastructure/Policies/ExponentialRetry/ExponentialRetryPolicy.cs +++ b/ShopifySharp/Infrastructure/Policies/ExponentialRetry/ExponentialRetryPolicy.cs @@ -36,14 +36,13 @@ ITaskScheduler taskScheduler } public async Task> Run( - CloneableRequestMessage requestMessage, + CloneableRequestMessage baseRequestMessage, ExecuteRequestAsync executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null) { var currentTry = 1; var useMaximumDelayBetweenRetries = false; - var clonedRequestMessage = requestMessage; var combinedCancellationToken = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); if (_options.MaximumDelayBeforeRequestCancellation is not null) @@ -53,6 +52,8 @@ public async Task> Run( { combinedCancellationToken.Token.ThrowIfCancellationRequested(); + using var clonedRequestMessage = await baseRequestMessage.CloneAsync(); + try { var value = await executeRequestAsync.Invoke(clonedRequestMessage); @@ -101,7 +102,6 @@ public async Task> Run( // Delay and then try again await _taskScheduler.DelayAsync(nextDelay, combinedCancellationToken.Token); - clonedRequestMessage = await requestMessage.CloneAsync(); } } } From 1b9af63913bc336152f188bf294f1425dff2ccfe Mon Sep 17 00:00:00 2001 From: Joshua Harms Date: Thu, 16 May 2024 11:01:05 -0500 Subject: [PATCH 2/2] Rename requestMessage parameter to baseRequestMessage --- .../TestClasses.cs | 2 +- .../Default/DefaultRequestExecutionPolicy.cs | 4 ++-- .../Policies/IRequestExecutionPolicy.cs | 4 ++-- .../LeakyBucket/LeakyBucketExecutionPolicy.cs | 14 +++++++------- .../Policies/Retry/RetryExecutionPolicy.cs | 4 ++-- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/ShopifySharp.Extensions.DependencyInjection.Tests/TestClasses.cs b/ShopifySharp.Extensions.DependencyInjection.Tests/TestClasses.cs index 3d0bd4302..0dce27ee3 100644 --- a/ShopifySharp.Extensions.DependencyInjection.Tests/TestClasses.cs +++ b/ShopifySharp.Extensions.DependencyInjection.Tests/TestClasses.cs @@ -8,7 +8,7 @@ public class TestException : Exception; public class TestRequestExecutionPolicy : IRequestExecutionPolicy { public Task> Run( - CloneableRequestMessage requestMessage, + CloneableRequestMessage baseRequestMessage, ExecuteRequestAsync executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null diff --git a/ShopifySharp/Infrastructure/Policies/Default/DefaultRequestExecutionPolicy.cs b/ShopifySharp/Infrastructure/Policies/Default/DefaultRequestExecutionPolicy.cs index 48b153691..dedc2306a 100644 --- a/ShopifySharp/Infrastructure/Policies/Default/DefaultRequestExecutionPolicy.cs +++ b/ShopifySharp/Infrastructure/Policies/Default/DefaultRequestExecutionPolicy.cs @@ -7,9 +7,9 @@ namespace ShopifySharp; public class DefaultRequestExecutionPolicy : IRequestExecutionPolicy { - public async Task> Run(CloneableRequestMessage request, ExecuteRequestAsync executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null) + public async Task> Run(CloneableRequestMessage baseRequestMessage, ExecuteRequestAsync executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null) { - var fullResult = await executeRequestAsync(request); + var fullResult = await executeRequestAsync(baseRequestMessage); return fullResult; } diff --git a/ShopifySharp/Infrastructure/Policies/IRequestExecutionPolicy.cs b/ShopifySharp/Infrastructure/Policies/IRequestExecutionPolicy.cs index c5540c60f..45d1d5adb 100644 --- a/ShopifySharp/Infrastructure/Policies/IRequestExecutionPolicy.cs +++ b/ShopifySharp/Infrastructure/Policies/IRequestExecutionPolicy.cs @@ -13,9 +13,9 @@ namespace ShopifySharp; /// public interface IRequestExecutionPolicy { - /// The base request that was built by a service to execute. + /// The base request that was built by a service to execute. /// A delegate that executes the request you pass to it. /// Cancellation token /// Optional expected Graphql query cost (as seen in GraphQL response at extensions.cost.requestedQueryCost - Task> Run(CloneableRequestMessage requestMessage, ExecuteRequestAsync executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null); + Task> Run(CloneableRequestMessage baseRequestMessage, ExecuteRequestAsync executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null); } diff --git a/ShopifySharp/Infrastructure/Policies/LeakyBucket/LeakyBucketExecutionPolicy.cs b/ShopifySharp/Infrastructure/Policies/LeakyBucket/LeakyBucketExecutionPolicy.cs index 95957b807..1a38a336f 100644 --- a/ShopifySharp/Infrastructure/Policies/LeakyBucket/LeakyBucketExecutionPolicy.cs +++ b/ShopifySharp/Infrastructure/Policies/LeakyBucket/LeakyBucketExecutionPolicy.cs @@ -74,14 +74,14 @@ public LeakyBucketExecutionPolicy(int maxRetriesPerNonLimitedRequest, bool retry _getRequestContext = getRequestContext ?? (() => RequestContext.Foreground); } - public async Task> Run(CloneableRequestMessage baseRequest, ExecuteRequestAsync executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null) + public async Task> Run(CloneableRequestMessage baseRequestMessage, ExecuteRequestAsync executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null) { - var accessToken = GetAccessToken(baseRequest); + var accessToken = GetAccessToken(baseRequestMessage); var bucket = accessToken == null ? null : _shopAccessTokenToLeakyBucket.GetOrAdd(accessToken, _ => new MultiShopifyApiBucket(_getRequestContext)); var apiType = ApiType.RestAdmin; - if (baseRequest?.RequestUri?.AbsolutePath.EndsWith("graphql.json") == true) - apiType = baseRequest.RequestUri.Host == "partners.shopify.com" ? ApiType.GraphQlPartner : ApiType.GraphQlAdmin; + if (baseRequestMessage?.RequestUri?.AbsolutePath.EndsWith("graphql.json") == true) + apiType = baseRequestMessage.RequestUri.Host == "partners.shopify.com" ? ApiType.GraphQlPartner : ApiType.GraphQlAdmin; return apiType switch { @@ -91,17 +91,17 @@ public async Task> Run(CloneableRequestMessage baseRequest, // If the user didn't pass a request query cost, we assume a cost of 50 graphqlQueryCost ?? 50, bucket, - baseRequest), + baseRequestMessage), ApiType.RestAdmin => await ExecuteRestAdminRequest( executeRequestAsync, cancellationToken, bucket, - baseRequest), + baseRequestMessage), ApiType.GraphQlPartner => await ExecuteGraphPartnerRequest( executeRequestAsync, cancellationToken, bucket, - baseRequest), + baseRequestMessage), _ => throw new ArgumentOutOfRangeException() }; } diff --git a/ShopifySharp/Infrastructure/Policies/Retry/RetryExecutionPolicy.cs b/ShopifySharp/Infrastructure/Policies/Retry/RetryExecutionPolicy.cs index 840e9f368..b517f9dd9 100644 --- a/ShopifySharp/Infrastructure/Policies/Retry/RetryExecutionPolicy.cs +++ b/ShopifySharp/Infrastructure/Policies/Retry/RetryExecutionPolicy.cs @@ -42,7 +42,7 @@ public RetryExecutionPolicy(bool retryOnlyIfLeakyBucketFull = true) } public async Task> Run( - CloneableRequestMessage baseRequest, + CloneableRequestMessage baseRequestMessage, ExecuteRequestAsync executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null @@ -53,7 +53,7 @@ public async Task> Run( while (true) { - using var request = await baseRequest.CloneAsync(); + using var request = await baseRequestMessage.CloneAsync(); try {