Skip to content
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.

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

ResiliencePipelineRegistry is now disposable #1496

Merged
merged 9 commits into from
Aug 17, 2023
Merged

Conversation

martintmk
Copy link
Contributor

@martintmk martintmk commented Aug 17, 2023

Details on the Issue Fix or Feature Implementation

The ResiliencePipeline can hold disposable resources such as RateLimiter or CircuitBreaker. This PR enables the ResiliencePipelineRegistry to manage and dispose of these resources transparently when necessary. Key changes include:

  • Dynamic Reloads: When a pipeline is reloaded, the previously unused one is disposed of.
  • DI Scenarios: Pipelines created using the AddResiliencePipeline DI extension are now seamlessly managed by both the registry and the DI infrastructure.
  • Registry Disposal: Disposing the ResiliencePipelineRegistry will also dispose of all associated pipelines and their resources.

I chose not to expose IDisposable and IAsyncDisposable on ResiliencePipeline as it compromises the API user experience (VS would flag the pipelines for disposal). Instead, we recommend using the registry or DI where such concerns are not present. We can add disposal support to ResiliencePipeline in the future, if there is a demand.

In standalone mode, as a workaround, users can manually dispose of the resources. Here's an example:

var limiter = new ConcurrencyLimiter(new ConcurrencyLimiterOptions { PermitLimit = 10, QueueLimit = 10 });
var pipeline = new ResiliencePipelineBuilder().AddRateLimiter(limiter).Build();
pipeline.Execute(() => { });
// Dispose of the limiter when the pipeline is no longer in use.
limiter.Dispose();

Additional Changes:

  • RateLimiterResilienceStrategy is now disposable.
  • CircuitBreakerResilienceStrategy is also disposable.
  • Improved thread safety for CircuitBreakerManualControl.
  • CircuitBreakerManualControl is no longer disposable.

This PR contributes to the discussion at #1233 (comment).

Confirm the Following:

  • I began this PR by branching from the head of the default branch.
  • I am targeting the PR to merge into the default branch.
  • I have included unit tests for the issue/feature.
  • I have successfully run a local build.

@martintmk martintmk added the v8 Issues related to the new version 8 of the Polly library. label Aug 17, 2023
@martintmk martintmk added this to the v8.0.0 milestone Aug 17, 2023
@martintmk martintmk self-assigned this Aug 17, 2023
@martintmk martintmk changed the title ResiliencePipelineRegistry is now disposable ResiliencePipelineRegistry is now disposable Aug 17, 2023
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #1496 (526dfb2) into main (46dc47f) will increase coverage by 0.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1496      +/-   ##
==========================================
+ Coverage   83.59%   83.88%   +0.29%     
==========================================
  Files         268      269       +1     
  Lines        6377     6492     +115     
  Branches     1007     1024      +17     
==========================================
+ Hits         5331     5446     +115     
  Misses        837      837              
  Partials      209      209              
Flag Coverage Δ
linux ?
macos 83.88% <100.00%> (+0.29%) ⬆️
windows 83.88% <100.00%> (+0.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/Polly.Core/ResiliencePipelineBuilderBase.cs 100.00% <ø> (ø)
...Core/CircuitBreaker/CircuitBreakerManualControl.cs 100.00% <100.00%> (ø)
...CircuitBreaker/CircuitBreakerResilienceStrategy.cs 100.00% <100.00%> (ø)
...ore/Registry/ResiliencePipelineRegistry.TResult.cs 100.00% <100.00%> (ø)
.../Polly.Core/Registry/ResiliencePipelineRegistry.cs 100.00% <100.00%> (ø)
src/Polly.Core/ResiliencePipeline.Async.cs 100.00% <100.00%> (ø)
src/Polly.Core/ResiliencePipeline.AsyncT.cs 100.00% <100.00%> (ø)
src/Polly.Core/ResiliencePipeline.Sync.cs 100.00% <100.00%> (ø)
src/Polly.Core/ResiliencePipeline.SyncT.cs 100.00% <100.00%> (ø)
src/Polly.Core/ResiliencePipeline.cs 100.00% <100.00%> (ø)
... and 10 more

@martintmk martintmk marked this pull request as ready for review August 17, 2023 07:57
@martintmk martintmk force-pushed the mtomka/dispose-support branch from deba4ce to bdbcb02 Compare August 17, 2023 10:50
@martintmk martintmk force-pushed the mtomka/dispose-support branch from bdbcb02 to d09f162 Compare August 17, 2023 10:52
@martintmk martintmk enabled auto-merge (squash) August 17, 2023 11:49
@martintmk martintmk merged commit 149ed82 into main Aug 17, 2023
@martintmk martintmk deleted the mtomka/dispose-support branch August 17, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 Issues related to the new version 8 of the Polly library.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants