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

When using HandleInner() not all AggregateExceptions are being checked #818

Closed
sideproject opened this issue Jan 13, 2021 · 4 comments · Fixed by #824
Closed

When using HandleInner() not all AggregateExceptions are being checked #818

sideproject opened this issue Jan 13, 2021 · 4 comments · Fixed by #824
Labels
Milestone

Comments

@sideproject
Copy link
Contributor

When handling exceptions that that are wrapped in an AggregateException, it looks like only the first exception is recursively searched for a matching predicate. If there are two (or more) exceptions in the InnerExceptions collection, they are not searched.

//Example Policy
var policy = Policy
    .HandleInner<DivideByZeroException>()
    .Retry(3, (exception, _) => passedToOnRetry = exception);
//The policy will find and retry the nested DivideByZeroException
Exception willRetryException = new AggregateException(
    new Exception("First: With Inner Exception"),
        new DivideByZeroException()));
    new Exception("Second: Without Inner Exception",
//The policy will NOT find and retry the nested DivideByZeroException
Exception willNotRetryException = new AggregateException(
    new Exception("First: Without Inner Exception"),
    new Exception("Second: With Inner Exception",
        new DivideByZeroException()));

I was able to make this work by modifying the HandleInner function in src/Polly/PolicyBuilder.OrSyntax.cs:55.

internal static ExceptionPredicate HandleInner(Func<Exception, bool> predicate)
{
    return exception =>
    {
        if (exception is AggregateException aggregateException)
        {
            foreach (var innerException in aggregateException.Flatten().InnerExceptions)
            {
                var matchedInAggregate = HandleInnerNested(predicate, innerException);
                if (matchedInAggregate != null)
                    return matchedInAggregate;
            }
        }

        return HandleInnerNested(predicate, exception);
    };
}

I'm happy to submit a pull request with tests for this if you're willing to entertain it.

@sideproject
Copy link
Contributor Author

Just checking in - is this change something that you'd entertain? I'm happy to submit a pull request for discussion.

@martincostello
Copy link
Member

This seems reasonable to me, but I'm not sure whether or not it could be considered a breaking behavioural change or not.

I guess if it's a minor enough code change to make you could open a PR for it and solicit further comment/opinion.

@sideproject
Copy link
Contributor Author

Understood, I've created PR #822 for your review.

The CI appears to have failed but doesn't seem to be related to my change - Could not load file or assembly 'YamlDotNet,

(Apologies for the second PR, I had some trouble with the first one so I started over)

sideproject added a commit to sideproject/Polly that referenced this issue Jan 24, 2021
sideproject added a commit to sideproject/Polly that referenced this issue Jan 25, 2021
sideproject added a commit to sideproject/Polly that referenced this issue Jan 25, 2021
@martincostello martincostello added this to the v7.2.2 milestone Jan 25, 2021
martincostello pushed a commit that referenced this issue Jan 25, 2021
…te matches when using HandleInner() (#824)

* Recursively search all AggregateException InnerExceptions for predicate matches when using HandleInner() #818

* Add tests for multiple nested aggregate exceptions #818
martincostello pushed a commit that referenced this issue Apr 11, 2021
…te matches when using HandleInner() (#824)

* Recursively search all AggregateException InnerExceptions for predicate matches when using HandleInner() #818

* Add tests for multiple nested aggregate exceptions #818
@martincostello
Copy link
Member

Fixed in v7.2.2.

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