这是indexloc提供的服务，不要输入任何密码
Skip to content

fix: Dispose cloned request messages in ExponentialRetryPolicy #1069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 16, 2024
Merged

fix: Dispose cloned request messages in ExponentialRetryPolicy #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b81e4d
fix: Dispose cloned request messages in ExponentialRetryPolicy
nozzlegear May 16, 2024
1b9af63
Rename requestMessage parameter to baseRequestMessage
nozzlegear May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ShopifySharp.Extensions.DependencyInjection.Tests/TestClasses.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ public class TestException : Exception;
public class TestRequestExecutionPolicy : IRequestExecutionPolicy
{
public Task<RequestResult<T>> Run<T>(
CloneableRequestMessage requestMessage,
CloneableRequestMessage baseRequestMessage,
ExecuteRequestAsync<T> executeRequestAsync,
CancellationToken cancellationToken,
int? graphqlQueryCost = null
Expand Down
13 changes: 13 additions & 0 deletions ShopifySharp.Tests/Infrastructure/Policies/ExponentialRetry/ExponentialRetryPolicyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<TestException>();

var policy = SetupPolicy();
var act = () => policy.Run(_cloneableRequestMessage, _executeRequest, CancellationToken.None);

await act.Should().ThrowAsync<TestException>();
_cloneableRequestMessage.Received(1).Dispose();
}

[Fact]
public async Task Run_ShouldThrowWhenRequestIsNotRetriableAsync()
{
Expand Down
4 changes: 2 additions & 2 deletions ShopifySharp/Infrastructure/Policies/Default/DefaultRequestExecutionPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ namespace ShopifySharp;

public class DefaultRequestExecutionPolicy : IRequestExecutionPolicy
{
public async Task<RequestResult<T>> Run<T>(CloneableRequestMessage request, ExecuteRequestAsync<T> executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null)
public async Task<RequestResult<T>> Run<T>(CloneableRequestMessage baseRequestMessage, ExecuteRequestAsync<T> executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null)
{
var fullResult = await executeRequestAsync(request);
var fullResult = await executeRequestAsync(baseRequestMessage);

return fullResult;
}
Expand Down
6 changes: 3 additions & 3 deletions ShopifySharp/Infrastructure/Policies/ExponentialRetry/ExponentialRetryPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,13 @@ ITaskScheduler taskScheduler
}

public async Task<RequestResult<T>> Run<T>(
CloneableRequestMessage requestMessage,
CloneableRequestMessage baseRequestMessage,
ExecuteRequestAsync<T> 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)
Expand All @@ -53,6 +52,8 @@ public async Task<RequestResult<T>> Run<T>(
{
combinedCancellationToken.Token.ThrowIfCancellationRequested();

using var clonedRequestMessage = await baseRequestMessage.CloneAsync();

try
{
var value = await executeRequestAsync.Invoke(clonedRequestMessage);
Expand Down Expand Up @@ -101,7 +102,6 @@ public async Task<RequestResult<T>> Run<T>(

// Delay and then try again
await _taskScheduler.DelayAsync(nextDelay, combinedCancellationToken.Token);
clonedRequestMessage = await requestMessage.CloneAsync();
}
}
}
4 changes: 2 additions & 2 deletions ShopifySharp/Infrastructure/Policies/IRequestExecutionPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ namespace ShopifySharp;
/// </summary>
public interface IRequestExecutionPolicy
{
/// <param name="requestMessage">The base request that was built by a service to execute.</param>
/// <param name="baseRequestMessage">The base request that was built by a service to execute.</param>
/// <param name="executeRequestAsync">A delegate that executes the request you pass to it.</param>
/// <param name="cancellationToken">Cancellation token</param>
/// <param name="graphqlQueryCost">Optional expected Graphql query cost (as seen in GraphQL response at extensions.cost.requestedQueryCost</param>
Task<RequestResult<T>> Run<T>(CloneableRequestMessage requestMessage, ExecuteRequestAsync<T> executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null);
Task<RequestResult<T>> Run<T>(CloneableRequestMessage baseRequestMessage, ExecuteRequestAsync<T> executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null);
}
14 changes: 7 additions & 7 deletions ShopifySharp/Infrastructure/Policies/LeakyBucket/LeakyBucketExecutionPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,14 @@ public LeakyBucketExecutionPolicy(int maxRetriesPerNonLimitedRequest, bool retry
_getRequestContext = getRequestContext ?? (() => RequestContext.Foreground);
}

public async Task<RequestResult<T>> Run<T>(CloneableRequestMessage baseRequest, ExecuteRequestAsync<T> executeRequestAsync, CancellationToken cancellationToken, int? graphqlQueryCost = null)
public async Task<RequestResult<T>> Run<T>(CloneableRequestMessage baseRequestMessage, ExecuteRequestAsync<T> 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
{
Expand All @@ -91,17 +91,17 @@ public async Task<RequestResult<T>> Run<T>(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()
};
}
Expand Down
4 changes: 2 additions & 2 deletions ShopifySharp/Infrastructure/Policies/Retry/RetryExecutionPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public RetryExecutionPolicy(bool retryOnlyIfLeakyBucketFull = true)
}

public async Task<RequestResult<T>> Run<T>(
CloneableRequestMessage baseRequest,
CloneableRequestMessage baseRequestMessage,
ExecuteRequestAsync<T> executeRequestAsync,
CancellationToken cancellationToken,
int? graphqlQueryCost = null
Expand All @@ -53,7 +53,7 @@ public async Task<RequestResult<T>> Run<T>(

while (true)
{
using var request = await baseRequest.CloneAsync();
using var request = await baseRequestMessage.CloneAsync();

try
{
Expand Down