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.

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

Add Outcome to the BreakDurationGeneratorArguments #2200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Outcome to the BreakDurationGeneratorArguments #2200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 17 additions & 9 deletions src/Polly.Core/CircuitBreaker/BreakDurationGeneratorArguments.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,31 +7,31 @@ namespace Polly.CircuitBreaker;
/// <summary>
/// Represents arguments used to generate a dynamic break duration for a circuit breaker.
/// </summary>
public readonly struct BreakDurationGeneratorArguments
/// <typeparam name="TResult">The type of result.</typeparam>
public readonly struct BreakDurationGeneratorArguments<TResult>
{
/// <summary>
/// Initializes a new instance of the <see cref="BreakDurationGeneratorArguments"/> struct.
/// Initializes a new instance of the <see cref="BreakDurationGeneratorArguments{TResult}"/> struct.
/// </summary>
/// <param name="failureRate">The failure rate at which the circuit breaker should trip.
/// It represents the ratio of failed actions to the total executed actions.</param>
/// <param name="failureCount">The number of failures that have occurred.
/// This count is used to determine if the failure threshold has been reached.</param>
/// <param name="context">The resilience context providing additional information
/// about the execution state and failures.</param>
/// <param name="outcome">The outcome of the resilience operation or event.</param>
[EditorBrowsable(EditorBrowsableState.Never)]
public BreakDurationGeneratorArguments(
double failureRate,
int failureCount,
ResilienceContext context)
ResilienceContext context,
Outcome<TResult> outcome)
: this(failureRate, failureCount, context, 0, outcome)
{
FailureRate = failureRate;
FailureCount = failureCount;
Context = context;
HalfOpenAttempts = 0;
}

/// <summary>
/// Initializes a new instance of the <see cref="BreakDurationGeneratorArguments"/> struct.
/// Initializes a new instance of the <see cref="BreakDurationGeneratorArguments{TResult}"/> struct.
/// </summary>
/// <param name="failureRate">The failure rate at which the circuit breaker should trip.
/// It represents the ratio of failed actions to the total executed actions.</param>
Expand All @@ -40,16 +40,19 @@ public BreakDurationGeneratorArguments(
/// <param name="context">The resilience context providing additional information
/// about the execution state and failures.</param>
/// <param name="halfOpenAttempts">The number of half-open attempts.</param>
/// <param name="outcome">The outcome of the resilience operation or event.</param>
public BreakDurationGeneratorArguments(
double failureRate,
int failureCount,
ResilienceContext context,
int halfOpenAttempts)
int halfOpenAttempts,
Outcome<TResult> outcome)
{
FailureRate = failureRate;
FailureCount = failureCount;
Context = context;
HalfOpenAttempts = halfOpenAttempts;
Outcome = outcome;
}

/// <summary>
Expand All @@ -71,4 +74,9 @@ public BreakDurationGeneratorArguments(
/// Gets the number of half-open attempts.
/// </summary>
public int HalfOpenAttempts { get; }

/// <summary>
/// Gets the outcome of the user-specified callback.
/// </summary>
public Outcome<TResult> Outcome { get; }
}
2 changes: 1 addition & 1 deletion src/Polly.Core/CircuitBreaker/CircuitBreakerStrategyOptions.TResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public class CircuitBreakerStrategyOptions<TResult> : ResilienceStrategyOptions
/// <value>
/// The default value is <see langword="null"/>.
/// </value>
public Func<BreakDurationGeneratorArguments, ValueTask<TimeSpan>>? BreakDurationGenerator { get; set; }
public Func<BreakDurationGeneratorArguments<TResult>, ValueTask<TimeSpan>>? BreakDurationGenerator { get; set; }

/// <summary>
/// Gets or sets a predicate that determines whether the outcome should be handled by the circuit breaker.
Expand Down
6 changes: 3 additions & 3 deletions src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ internal sealed class CircuitStateController<T> : IDisposable
private readonly ResilienceStrategyTelemetry _telemetry;
private readonly CircuitBehavior _behavior;
private readonly TimeSpan _breakDuration;
private readonly Func<BreakDurationGeneratorArguments, ValueTask<TimeSpan>>? _breakDurationGenerator;
private readonly Func<BreakDurationGeneratorArguments<T>, ValueTask<TimeSpan>>? _breakDurationGenerator;
private DateTimeOffset _blockedUntil;
private CircuitState _circuitState = CircuitState.Closed;
private Outcome<T>? _lastOutcome;
Expand All @@ -33,7 +33,7 @@ public CircuitStateController(
CircuitBehavior behavior,
TimeProvider timeProvider,
ResilienceStrategyTelemetry telemetry,
Func<BreakDurationGeneratorArguments, ValueTask<TimeSpan>>? breakDurationGenerator)
Func<BreakDurationGeneratorArguments<T>, ValueTask<TimeSpan>>? breakDurationGenerator)
#pragma warning restore S107
{
_breakDuration = breakDuration;
Expand Down Expand Up @@ -323,7 +323,7 @@ private void OpenCircuitFor_NeedsLock(Outcome<T> outcome, TimeSpan breakDuration
{
#pragma warning disable CA2012
#pragma warning disable S1226
breakDuration = _breakDurationGenerator(new(_behavior.FailureRate, _behavior.FailureCount, context, _halfOpenAttempts)).GetAwaiter().GetResult();
breakDuration = _breakDurationGenerator(new(_behavior.FailureRate, _behavior.FailureCount, context, _halfOpenAttempts, outcome)).GetAwaiter().GetResult();
#pragma warning restore S1226
#pragma warning restore CA2012
}
Expand Down
19 changes: 10 additions & 9 deletions src/Polly.Core/PublicAPI.Shipped.txt
View file
Open in desktop
Copy link
Member

@martincostello martincostello Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New entries should go in the Unshipped file. What you're doing here is effectively "tricking" the Public API analyser into thinking breaking changes aren't.

Copy link
Contributor Author

@peter-csala peter-csala Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's quite unfortunate. :(

That would require me to create a BreakDurationGeneratorArguments.TResult.cs. Which is fine but inside the CircuitBreakerStrategyOptions.TResult.cs I have to create a new generator to make this change backward compatible.

 public Func<BreakDurationGeneratorArguments, ValueTask<TimeSpan>>? BreakDurationGenerator { get; set; }

 public Func<BreakDurationGeneratorArguments<TResult>, ValueTask<TimeSpan>>? NewBreakDurationGenerator { get; set; }

Do you have any idea how to solve this problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not easily, no - is what you're trying to do achievable via storing the result in the context?

Copy link
Contributor Author

@peter-csala peter-csala Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OnOpened is called after the break duration calculation, because the OnCircuitOpenedArguments contains the break duration as well.

So, I can't set the context from the OnOpened that's why I have to set it inside the user callback. Which is not possible if I decorate my HttpClient via the AddResilienceHandler. :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @martintmk has some ideas.

If the type was a class we could just inherit from it and add a generic version, the we wouldn't need to touch the generator but you'd need to do a cast /as to get the generic version. As it's a struct we can't do that.

I don't think introducing a "NewBreakDurationGenerator" parallel dimension would be a good move and I don't think we'd want to go down the route of a v9 to make this kind of breaking change any time soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this NewBreakDurationGenerator idea is non-sense. I've just used it to illustrate the problem.

If my memory serves well this is the 3rd or 4th time when we do not fix something on the API due to backward compatibility. Would it make sense to collect these under a ticket and label it with v9?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could maybe use the existing breaking change label and park an issues that would be nice to have but would be breaking there.

Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@ override Polly.Registry.ResiliencePipelineRegistry<TKey>.TryGetPipeline(TKey key
override Polly.Registry.ResiliencePipelineRegistry<TKey>.TryGetPipeline<TResult>(TKey key, out Polly.ResiliencePipeline<TResult>? pipeline) -> bool
override Polly.ResiliencePropertyKey<TValue>.ToString() -> string!
override Polly.Telemetry.ResilienceEvent.ToString() -> string!
Polly.CircuitBreaker.BreakDurationGeneratorArguments
Polly.CircuitBreaker.BreakDurationGeneratorArguments.BreakDurationGeneratorArguments() -> void
Polly.CircuitBreaker.BreakDurationGeneratorArguments.BreakDurationGeneratorArguments(double failureRate, int failureCount, Polly.ResilienceContext! context) -> void
Polly.CircuitBreaker.BreakDurationGeneratorArguments.BreakDurationGeneratorArguments(double failureRate, int failureCount, Polly.ResilienceContext! context, int halfOpenAttempts) -> void
Polly.CircuitBreaker.BreakDurationGeneratorArguments.Context.get -> Polly.ResilienceContext!
Polly.CircuitBreaker.BreakDurationGeneratorArguments.FailureCount.get -> int
Polly.CircuitBreaker.BreakDurationGeneratorArguments.FailureRate.get -> double
Polly.CircuitBreaker.BreakDurationGeneratorArguments.HalfOpenAttempts.get -> int
Polly.CircuitBreaker.BreakDurationGeneratorArguments<TResult>
Polly.CircuitBreaker.BreakDurationGeneratorArguments<TResult>.BreakDurationGeneratorArguments() -> void
Polly.CircuitBreaker.BreakDurationGeneratorArguments<TResult>.BreakDurationGeneratorArguments(double failureRate, int failureCount, Polly.ResilienceContext! context, Polly.Outcome<TResult> outcome) -> void
Polly.CircuitBreaker.BreakDurationGeneratorArguments<TResult>.BreakDurationGeneratorArguments(double failureRate, int failureCount, Polly.ResilienceContext! context, int halfOpenAttempts, Polly.Outcome<TResult> outcome) -> void
Polly.CircuitBreaker.BreakDurationGeneratorArguments<TResult>.Context.get -> Polly.ResilienceContext!
Polly.CircuitBreaker.BreakDurationGeneratorArguments<TResult>.FailureCount.get -> int
Polly.CircuitBreaker.BreakDurationGeneratorArguments<TResult>.FailureRate.get -> double
Polly.CircuitBreaker.BreakDurationGeneratorArguments<TResult>.HalfOpenAttempts.get -> int
Polly.CircuitBreaker.BreakDurationGeneratorArguments<TResult>.Outcome.get -> Polly.Outcome<TResult>
Polly.CircuitBreaker.BrokenCircuitException
Polly.CircuitBreaker.BrokenCircuitException.BrokenCircuitException() -> void
Polly.CircuitBreaker.BrokenCircuitException.BrokenCircuitException(string! message) -> void
Expand All @@ -41,7 +42,7 @@ Polly.CircuitBreaker.CircuitBreakerStrategyOptions.CircuitBreakerStrategyOptions
Polly.CircuitBreaker.CircuitBreakerStrategyOptions<TResult>
Polly.CircuitBreaker.CircuitBreakerStrategyOptions<TResult>.BreakDuration.get -> System.TimeSpan
Polly.CircuitBreaker.CircuitBreakerStrategyOptions<TResult>.BreakDuration.set -> void
Polly.CircuitBreaker.CircuitBreakerStrategyOptions<TResult>.BreakDurationGenerator.get -> System.Func<Polly.CircuitBreaker.BreakDurationGeneratorArguments, System.Threading.Tasks.ValueTask<System.TimeSpan>>?
Polly.CircuitBreaker.CircuitBreakerStrategyOptions<TResult>.BreakDurationGenerator.get -> System.Func<Polly.CircuitBreaker.BreakDurationGeneratorArguments<TResult>, System.Threading.Tasks.ValueTask<System.TimeSpan>>?
Polly.CircuitBreaker.CircuitBreakerStrategyOptions<TResult>.BreakDurationGenerator.set -> void
Polly.CircuitBreaker.CircuitBreakerStrategyOptions<TResult>.CircuitBreakerStrategyOptions() -> void
Polly.CircuitBreaker.CircuitBreakerStrategyOptions<TResult>.FailureRatio.get -> double
Expand Down
8 changes: 6 additions & 2 deletions test/Polly.Core.Tests/CircuitBreaker/BreakDurationGeneratorArgumentsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ public void Constructor_Old_Ok()
double expectedFailureRate = 0.5;
int failureCount = 10;
var context = new ResilienceContext();
var outcome = Outcome.FromResult(42);

var args = new BreakDurationGeneratorArguments(expectedFailureRate, failureCount, context);
var args = new BreakDurationGeneratorArguments<int>(expectedFailureRate, failureCount, context, outcome);

args.FailureRate.Should().Be(expectedFailureRate);
args.FailureCount.Should().Be(failureCount);
args.Context.Should().Be(context);
args.Outcome.Should().Be(outcome);
}

[Fact]
Expand All @@ -25,12 +27,14 @@ public void Constructor_Ok()
double expectedFailureRate = 0.5;
int failureCount = 10;
var context = new ResilienceContext();
var outcome = Outcome.FromResult(42);

var args = new BreakDurationGeneratorArguments(expectedFailureRate, failureCount, context, 99);
var args = new BreakDurationGeneratorArguments<int>(expectedFailureRate, failureCount, context, 99, outcome);

args.FailureRate.Should().Be(expectedFailureRate);
args.FailureCount.Should().Be(failureCount);
args.Context.Should().Be(context);
args.HalfOpenAttempts.Should().Be(99);
args.Outcome.Should().Be(outcome);
}
}
Loading