Makes cancellable configuration-providing delegates

[vany0114]

@reisenberger I updated the overloads which were taking the Context in configuration-providing delegates instead of adding new overloads (breaking change), basically to avoid the proliferation of the overloads as we discussed it previously, but it might be solved with the new syntax tho.

I added some validations on AsyncMonkeyEngine (not sure if that is correct) in order to avoid to execute extra code when the cancellation token is signaled at different times during the execution.

• Before the execution starts
• When the user cancels execution on enable configuration delegate
• When the user cancels execution on injectionRate configuration delegate
• When a user cancels execution on injectedBehaviour delegate (This is not a configuration delegate but the user might want to cancel the original action after to inject a custom behavior)
• When user cancels execution on injectedException configuration delegate

I also changed the AsyncInjectLatencyPolicy (1, 2) in order to make the latency configuration delegate cancellable because it was injecting the latency even though you signal the cancellation from that configuration delegate.

However, after this refactor I have a big doubt: should we let the cancellation token request flow to the user rather than let Simmy does it using ThrowIfCancellationRequested() on AsyncMonkeyEngine? if not, I'll need to update the MonkeyEngine as well (to put those validations).

Or just validate once on ShouldInjectAsync method at the beginning? and let the user does something like this:

Func<Context, CancellationToken, Task> actionOrConfigDelegateAsync = (ctx, ct) =>
{
     ct.ThrowIfCancellationRequested();
     ...
};

Thoughts?

ref #19

[reisenberger]

Thanks @vany0114 , these are great questions! I will try to dive in and have a look over the weekend/coming days!

[reisenberger]

👀 looking very nice to me, @vany0114 ! Will contribute thoughts separately on questions in your cover note.

[reisenberger]

👍 Perfect for a policy engine to test for cancellation as the first thing it does. This is a typical pattern for Polly policies.

It matches the approach of the .NET base class libraries: for example Task.Run(...) makes a cancellation check before running the passed delegate.

[reisenberger]

👍 to check for cancellation again here (in case the CancellationToken has been signalled, but the user's enabled delegate didn't check it, so didn't honor cancellation).

Note: the good reason for checking cancellation here is that a timing-out CancellationToken might have been provided (for example from a TimeoutPolicy), and it is possible the user's enabled delegate didn't honor it.
(I am not thinking so much that the user would have signal the CancellationToken within the enabled delegate. If the user wants the chaos not to go ahead, they can simply return false from that delegate.)

[vany0114]

Agree, I think that's gonna cover cancellation for timing out just in case the user wrap chaos policies with a timeout policy, which would be highly likely.

[reisenberger]

👍 to check for cancellation again here (for same reason as a few lines above)

[reisenberger]

Looks good to me! I expect, if the user wanted to choose not to introduce the chaos, they could just return false from this delegate. But the cts.Cancel() here is a good simulation of something else (eg a TimeoutPolicy) signalling the CancellationToken some time during the execution of the enabled delegate.

(Similarly for all the other tests.)

[vany0114]

Exactly, I think is very unlikely that a user does something like that explicitly to cancel the chaos injection, I just wanted to show how would be the behavior of cancellation when the token is signaled, usually (as you said) from a timeout policy. Maybe I should rename the test cases to be clearer 😅

[reisenberger]

Hey @vany0114

updated the overloads [...] instead of adding new overloads (breaking change), basically to avoid the proliferation

👍

added some validations on AsyncMonkeyEngine

Looks good to me! Per my comments in detailed review, a good reason for checking for cancellation frequently can be because something independent (some other thread; a timeout timer) has signalled the CancellationToken. And because the user's delegate may not have honored that CancellationToken (if they didn't bother to code it to do so).

should we let the cancellation token request flow to the user rather than let Simmy does it using ThrowIfCancellationRequested() on AsyncMonkeyEngine?

I think it is fine for a Policy to enforce cancellation even if the user code doesn't. Polly policies do this.

---

You are right that we should make the sync policies do the same.

After that, you can decide whether you want to make a v0.2 release with this straight away 🚀 , or if there is anything else you want to add/change for a v0.2. I can talk you through the process we normally use (for Polly) when we make a new release!

[vany0114]

Hey @reisenberger

Thanks for taking a look and your thoughts. Yeah, I'm gonna do the change for sync policies as well and also implement #20 since it's related too. After that, I can make a v0.2 release.

And yes, I'd love to know how you guys handle the release process on Polly to apply it on Simmy as well!

[vany0114]

Sync policies are done already. I've updated the PR.