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

Implement BeforeInject callback #66

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

Conversation

iCodeSometime
Copy link

Closes #65

@vany0114 vany0114 self-requested a review January 21, 2022 01:10
Copy link
Member

@vany0114 vany0114 left a comment

Choose a reason for hiding this comment

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

@iCodeSometime thanks for taking the time to open this PR, it looks great! I just left a few minor comments regarding honoring the cancellation token, said that, would be nice to add some tests to cover that scenario as well, you can see some examples in the cancellable scenarios. And BTW nice job with the unit tests you added!

I wonder if we should add the AfterInject API as well 🤔 I think would be nice to give the users both options, I can imagine they might want to take actions before and/or after the chaos is injected.

bool continueOnCapturedContext)
{
if (await ShouldInjectAsync(context, cancellationToken, injectionRate, enabled, continueOnCapturedContext).ConfigureAwait(continueOnCapturedContext))
{
if (beforeInjectCallback != null)
{
await beforeInjectCallback(context, cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

now that we're allowing injecting a delegate right before injecting the chaos, I think we should honor the token, just in case the user signaled the token from there:

cancellationToken.ThrowIfCancellationRequested();

@@ -71,6 +77,10 @@ internal static class AsyncMonkeyEngine

if (exception != null)
{
if (beforeInjectCallback != null)
{
await beforeInjectCallback(context, cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

bool continueOnCapturedContext)
{
if (await ShouldInjectAsync(context, cancellationToken, injectionRate, enabled, continueOnCapturedContext).ConfigureAwait(continueOnCapturedContext))
{
if (beforeInjectCallback != null)
{
await beforeInjectCallback(context, cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

{
if (ShouldInject(context, cancellationToken, injectionRate, enabled))
{
if (beforeInjectCallback != null)
{
beforeInjectCallback(context, cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -63,6 +69,10 @@ private static bool ShouldInject(Context context, CancellationToken cancellation

if (exception != null)
{
if (beforeInjectCallback != null)
{
beforeInjectCallback(context, cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

{
if (ShouldInject(context, cancellationToken, injectionRate, enabled))
{
if (beforeInjectCallback != null)
{
beforeInjectCallback(context, cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

with.Behaviour(async () =>
{
beforeInjectExecuted.Should().BeTrue();
injectedBehaviourExecuted = true;
Copy link
Member

Choose a reason for hiding this comment

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

make sure to either remove the async keyword from these delegates or await an EmptyTask so that the compiler doesn't complain about the lack of the await keyword:

var policy = MonkeyPolicy.InjectBehaviourAsync(with =>
                with.Behaviour(async () =>
                {
                    beforeInjectExecuted.Should().BeTrue();
                    injectedBehaviourExecuted = true;
                    await TaskHelper.EmptyTask;
                })
                .BeforeInject(async (context, cancellation) =>
                {
                    injectedBehaviourExecuted.Should().BeFalse();
                    beforeInjectExecuted = true;
                    await TaskHelper.EmptyTask;
                })
                .InjectionRate(0.6)
                .Enabled()
            );

Note: this applies to all the tests you added, you can see the warnings in the build: https://ci.appveyor.com/project/Polly-Contrib/simmy/builds/42270091

Copy link
Author

Choose a reason for hiding this comment

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

Thank you. I did see the warnings, but haven't had time to address them yet.

@iCodeSometime
Copy link
Author

Thank you for the feedback. Expecting to have time to clean this up in about a week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants