Polly icon indicating copy to clipboard operation
Polly copied to clipboard

Revise Avoid compiler gotchas mixing sync and async execution

Open ndrwrbgs opened this issue 2 years ago • 1 comments

Is your feature request related to a specific problem? Or an existing feature? Please describe. Specific problem. A user can call Policy.Execute on a Func<Task> without any signal they've done so. They may not notice the issue until they have a real issue that was not retrying as expected.

Describe your proposed or preferred solution: RE: https://github.com/App-vNext/Polly/wiki/Avoid-compiler-gotchas-mixing-sync-and-async-execution I think we could introduce a Roslyn analyzer packaged with Polly that could detect this and raise the issue to the developer.

Describe any alternative options you've considered: Alternatively, I could see us introducing an overload to signal with an ObsoleteAttribute, though that may be too heavy-handed for users (CS0618 is a major analyzer to fail, and technically the mechanism isn't obsolete) -- note this approach we would need to handle every Execute overload.

[Obsolete("Use an AsyncPolicy and ExecuteAsync for async retries")]
public static Task Execute(
    this Policy policy,
    Func<Task> action) => policy.Execute(action);
[Obsolete("Use an AsyncPolicy and ExecuteAsync for async retries")]
public static Task<TResult> Execute<TResult>(
    this Policy policy,
    Func<Task<TResult>> action) => policy.Execute(action);

Any additional info? We should update the linked wiki if changed. There may be cases where a user actually does only want to retry the sync (Task creation) part of an operation.

ndrwrbgs avatar Nov 17 '21 06:11 ndrwrbgs

The following seems to be appx what we'd need in an Analyzer

        public override void Initialize(AnalysisContext context)
        {
            // ...
            context.RegisterSyntaxNodeAction(AnalyzeSyntax, SyntaxKind.InvocationExpression);
        }
        
        private void AnalyzeSyntax(SyntaxNodeAnalysisContext context)
        {
            var invocationExpression = (InvocationExpressionSyntax)context.Node;
            // TODO: Filter as much as we can on the syntax to reduce overhead - or use the SemanticModel Register hook?
            var symbol = context.SemanticModel.GetSymbolInfo(invocationExpression).Symbol as IMethodSymbol;
            
            if (symbol == null) return;

            // Internet has better ways of comparing the INamedTypeSymbol to a Type
            if (string.Equals(symbol.ContainingType?.ToString(), "Polly.Policy", StringComparison.Ordinal))
            {
                if (symbol.ContainingType.ContainingAssembly?.Identity?.Name?.Contains("Polly") == true)
                {
                    if (string.Equals(symbol.Name, "Execute", StringComparison.Ordinal))
                    {
                        var typeArg = symbol.TypeArguments.SingleOrDefault();
                        if (typeArg == null) return; // not match

                        if (typeArg.ToString().StartsWith("System.Threading.Tasks.Task", StringComparison.Ordinal))
                        {
                            // This means they called Policy.Execute with a Task or Task{T}
                            context.ReportDiagnostic(
                                Diagnostic.Create(
                                    Rule,
                                    context.Node.GetLocation(),
                                    typeArg.ToString()));
                        }
                    }
                }
            }
        }

(needs 'productionization')

ndrwrbgs avatar Nov 17 '21 06:11 ndrwrbgs

I believe this use case is improved in Polly v8 and doesn't require a custom analyzer.

martincostello avatar Jun 16 '23 15:06 martincostello