DecorrelatedJitter is a first-class citizen

[grant-d]

ISleepDurationSeriesStrategy providers

Per my design proposal in #526, this PR includes changes that make DecorrelatedJitterBackoff, ConstantBackoff, LinearBackoff and ExponentialBackoff first-class citizens in Polly.

Questions

Details in the linked issue.

• Any other backoff strategies we need [Edit: No]
• Is the interface sufficient [Edit: Trimmed]
• I have added one overload to PolicyBuilder to demonstrate the pattern should we decide to integrate it directly. See additional notes there. I am happy to revert such integration if need be. [Edit: Scoped-out]
• What about the *Forever overloads, I have not integrated that at all. [Edit: Scoped-out]

Notes

• The shared Random instance is lock controlled (per separate discussion)
• I have made lots of comments on the CR to explain what's going on
• Please forgive a small amount of reformatting - my VS setup is configured to do it automatically. I will revert if necessary.

Usage

Details are in the associated design document. But for quick reference, here's how you would call a strategy. Of course, we may decide to provide direct overloads too:
Here's an example of how to call it. Note that the class is instantiated as a singleton (directly or via DI) and each execution uses the .Generate().

private static readonly ISleepDurationStrategy s_backoff = new DecorrelatedJitterBackoff(MinDelay, MaxDelay);

return Policy<IRestResponse<T>>
    .Handle<HttpRequestException>()
    .OrResult(resultPredicate)
    .WaitAndRetryAsync(s_backoff.Generate(retryCount: 5)) // <-- Note usage
    .ExecuteAsync(() => client.ExecuteTaskAsync<T>(request, cancellationToken));

Risks

• Mitigated: the use of yield instead of an eager TimeSpan[] array has mitigated the following concern: Inadequate guidance could lead to consumers using the exact same instance of a strategy across multiple calls. It is essential that .Generate is called just-in-time, not pre-generated. (Docs should include info in DecorrelatedJitter is a first-class citizen #536 (comment))

Confirm the following

(I am not sure which of the 2 dev branches to merge from and target) EDIT by @reisenberger : I fixed this and targeted the branch.

• I have merged the latest changes from the dev vX.Y branch
• I have successfully run a local build
• I have included unit tests for the issue/feature
• I have targeted the PR against the latest dev vX.Y branch

[grant-d]

can be enumerated multiple times, giving different randomised results …

I thought about this some more. I think the above would only work in serializable contexts. What would be our guidance when users try to use a shared (eg reified singleton) strategy in concurrent scenarios. In that case you'd need a distinct enumerator per context.

[reisenberger]

can be enumerated multiple times, giving different randomised results …

I think the above would only work in serializable contexts

IIRC the details of yield return implementation, this will be fine whether different enumerations of the enumerable are executing serially or in parallel/concurrently. When the compiler sees a method returning IEnumerable which is implemented with yield return, it (IIRC) compiler-generates a hidden inner class to represent that method returning IEnumerable. Each enumeration instantiates a separate instance of that compiler-generated inner-class. (This actually gives the thing you rightly identify is needed: "a distinct enumerator per [execution]".) That's a quick explanation from memory; blogs should confirm.

So it all works thread-safely - provided, of course, that within our IEnumerable<T>-returning, yield return-implemented method, we don't use anything non-thread-safely. Everything mutable within DecorrelatedJitter.GetSleepDurations(...) has method-execution-scope, and we take care that we use the only shared/external item, _random, thread-safely, so it looks good to me.

[grant-d]

this will be fine whether different enumerations of the enumerable are executing serially or in parallel/concurrently ...

You are right, I couldn't recall whether the generated state machine was per-execution or not. I convinced myself using the following code (and associated unit):

            const int count = 10;
            DecorrelatedJitterBackoff durationStrategy = new DecorrelatedJitterBackoff(TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(3));

            IEnumerable<TimeSpan> generate = durationStrategy.GetSleepDurations(count);

            var tasks = new Task<TimeSpan[]>[100];
            var start = new ManualResetEvent(false);

            for (var i = 0; i < tasks.Length; i++)
            {
                var tcs = new TaskCompletionSource<TimeSpan[]>();
                tasks[i] = tcs.Task;

                var thread = new Thread(() =>
                {
                    start.WaitOne();
                    tcs.SetResult(generate.ToArray()); // Every thread enumerates the singleton
                });
                thread.Start();
            }

            start.Set(); // Unleash the fury
            Task.WaitAll(tasks);

            tasks.Should().NotContain(n => n.IsFaulted); // All passed

[reisenberger]

@grant-d @george-polevoy . Similar to for the current fault-injection work, I am going to propose that we promote this contribution to its own package, in this case (something like) Polly.Contrib.WaitAndRetryStrategies. Advantages:

• More freedom to rev Polly.Contrib.WaitAndRetryStrategies independently from (not blocked by) the Polly rev cycle - grow/refine more freely as enhancements to the algorithms occur to us/anyone
• We can include more of @george-polevoy 's algorithm variants (if George is happy to)
• We can give you greater control over the Polly.Contrib package (if you wish); and headlining as contributors (either way)
• More space to document the math in depth

Please let me know any thoughts.

Mechanics:

• I would need time to set up the separate package.
• We would likely first merge, then-demerge, this PR on Polly in any case, to ensure you get the due contributor credit
• We would advertise the package directly from the Polly retry wiki page (and everywhere else relevant)

[reisenberger]

@grant-d The separate package proposal (if agreed) helps guide some of our decisions around interfaces-or-not; and instances-of-a-class configuration versus static factory methods. Propose we may go for:

(a) static factory methods on a separate-class-per-strategy, ie a syntax DecorrelatedJitter.GetSleepDurations(param1, param2, retryCount) , LinearBackoff.GetSleepDurations(....), etc.

• static class-per-strategy grows more easily than one class holding all variants

(b) static methods, no instances, no interface

• IEnumerable<TimeSpan> seems a clear and well enough defined interface between Polly.Contrib.WaitAndRetryStrategies and Polly

• Less cruft, as you say. Similarly, I have tended to avoid introducing configuration instance classes to Polly, despite requests, to avoid it becoming extra surface to maintain, constraining the API (adding options breaks the classes), and it is easy enough for Polly users to do themselves in the particular way they want anyway.

Again, let me know if this path forward seems ok.

[grant-d]

Sorry, been snowed under at work, will try get to this this week

[reisenberger]

@grant-d No problem!

[grant-d]

Sorry for the long delay, I should have more time now.
I have implemented the factory methods as discussed, including sample units so you can get a feel for the call-site shape.
Your a) vs b) choice is interesting. Considering the semantics of IEnumerable, it may be sufficient to go with b) for what we need. Would you still propose a separate (static) class per strategy?

[grant-d]

Regarding core vs contrib. I don't mind either way except for the risk that other contribs I have used in the past have become ill-maintained and/or runaway trains, filled with everyone's favorite idea. As long as we curate both repos strictly, I think we should be fine.

[grant-d]

Here's the option where we have static methods returning IEnumerable only

[grant-d]

Sample callsite for static instantiation

[grant-d]

Sample callsite for factory instantiation

[grant-d]

Here is the new factory method

[reisenberger]

Thanks @grant-d . I disposed of a question around the new jitter strategy also over the festive season, probably going to be a bit more kanban over next couple of weeks (courtesy update) to focus purely on getting Polly v7.0 and Simmy out, then let's bring this to fruition also. Thanks again!

[reisenberger]

This PR was unexpectedly automatically closed by github, when deleting a (now-outdated) base branch which the PR targeted .

@grant-d Thank you for everything you have done on this! Now that Polly v7.0.0 is released, I hope to revert shortly to how we may take this forward.

[grant-d]

@reisenberger any news on the plans here

[reisenberger]

@grant-d , I hope to come back to this at the weekend.

[grant-d]

Cool. My personal feeling is that class called Alternative is the best approach. I have used such a pattern with Polly in my own code recently, and it's clean.