Current PollyNator output may not play well for Interface containing both sync and async overloads

[reisenberger]

Have reviewed the functionality of Pollynator (love the name) as described in the readme. One potential wrinkle: Polly currently requires separate policy instances for sync and async executions.

From the readme, PollyNator generates:

    private readonly ITestInterface _wrappedImplemention;
    private readonly PolicyWrap _polly;

    TestInterfaceClass(ITestInterface implementation, PolicyWrap polly)
    {
        _wrappedImplemention = implementation;
        _polly = polly;
    }

If ITestInterface contains both sync and async methods, a single PolicyWrap polly cannot be used to execute both: either the sync or the async executions will throw.

(This is a less-than-ideal design element in Polly, dating from prior to App-vNext stewardship. The blog post discusses factors affecting. A proposal within Polly seeks views whether to change this.)

---

Given this restriction in current Polly, PollyNator could:

• if the interface-to-be-decorated contains only sync methods, generate a ctor taking PolicyWrap policy
• if the interface-to-be-decorated contains only async methods, generate a ctor taking PolicyWrap asyncPolicy
• if the interface-to-be-decorated contains both sync and async methods, generate a ctor taking PolicyWrap syncPolicy, PolicyWrap asyncPolicy

(only thoughts - you may have other ideas!)

[mcquiggd]

@reisenberger Actually I had actually originally created a constructor that accepted two different Policy types, and then each of the helper methods tested if one was null, and executed the one that wasn't, so such code is perfectly possible to generate.

It is also possible to test if all methods return a Task, or not.

It does make the resulting code less... elegant... to use, but, as that is the current state of play with Polly, then I will update Pollynator to support these scenarios.

[mcquiggd]

Thinking aloud, mostly for my own benefit (I am the type of developer who draws a lot when looking at code, and this is the next best thing)....

Logic to implement as per current discussion

        // Polly currently requires different instances to handle sync and async methods.
        // 1. Determine if there are any async methods. 
        //      If so:
        //          We must include an *async* Policy instance parameter in the constructor
        //          We must also include an *async* specific instance field.
        //          We should include the *ExecuteAsync* method, and delegate all calls to the *async* specific instance field.
        //      If not:
        //          There is no need to include the above.
        // 2. Determine if all methods are async
        //      If so: 
        //          There is no need to add a sync Policy instance parameter in the constructor
        //      If not:
        //          We must include an *sync* Policy instance parameter in the constructor
        //          We must also include an *sync* specific instance field.
        //          We should include the *Execute* method, and delegate all calls to the *sync* specific instance field.

A consideration:

Problem - Dealing with changes to APIs.

Regenerating code if an interface changes (e.g. an async method is introduced, where one was not present before, or, all methods becoming async), will result in adjusting the signature of an existing constructor. It would also require adding or removing a helper method, that could have been customised. Not rocket science, but does create some spaghetti in the Pollynator codebase.

Such changes could become confusing to the developer, and bearing in mind the current discussion on unifying the sync and async approaches in Polly, it would soon (timescale?) be redundant anyway.

Alternative approach 1

Assumption: Most methods will be async.

To avoid having additional logic that adds and removes code, and planning for the 'vNext' Polly, adopt the following approach:

1. Always include a constructor that accepts polly, and pollySync, with the latter being nullable.

2. Add code comments explaining under which circumstances the two parameters are used.

3. Always include the sync and async helper methods. It's minimal code, and has no impact. The sync helper methods call the pollySync instance.

4. If a sync method is called, and the corresponding pollySync instance is null, throw a descriptive error, informing the developer that they must pass in a sync instance.

5. When Polly vNext is released, remove the pollySync parameter, and change the sync helper function to call polly.

Alternative approach 2

Assumption: Most methods will be async.

1. Keep the existing constructor, and if sync methods are present, include an additional constructor that accepts polly, and pollySync. (Should this be removed later if all methods become async?)

2. Add code comments explaining under which circumstances the two constructors are used.

3. Always include the sync and async helper methods. It's minimal code, and has no impact. The sync helper methods call the pollySync instance.

4. If a sync method is called, and the corresponding pollySync instance is null, throw a descriptive error, informing the developer that they must pass in a sync instance.

5. When Polly vNext is released, remove the redundant constructor, and change the sync helper function to call polly.

Alternative approach 3

Assumption: Most methods will be async.

1. Always include two constructors; constructor 1 accepts polly, constructor 2 accepts pollySync and pollyAsync.

2. Add code comments explaining under which circumstances the two constructors are used. e.g. version of Polly that is utilised.

3. Always include the sync and async helper methods. It's minimal code, and has no impact. The helper methods contain simple if statements to determine which Polly instance to use.

4. If a helper method is called, and the corresponding instance is null, throw a descriptive error, informing the developer that they must pass in the required instance to the relevant constructor.

5. When Polly vNext is released, remove the redundant constructor (or comment out, or mark as obsolete), and change the sync helper function to call polly.

Alternative approach 4

Mark this project as a beta work in progress, pending Polly vNext, depending on ETA.

Personally, I am leaning towards Alternative Approach 3. Chopping and changing existing code would undermine the simplicity of Pollynator, and make future enhancements problematic. Option 3 allows support for current, and vNext, and the removal of code to support legacy versions can be notified to users in advance. Releases will be archived here on GitHub, and latest releases will be published in Visual Studio Marketplace with appropriate readme.

Final decision to be made after further discussion on App-vNext/Polly#281

NOTE TO SELF: Examine using Roslyn to comment out existing methods rather than altering / removing them, to preserve customisations. Also look at adding an Obsolete attribute to methods during generation / regeneration.

[reisenberger]

Hi @mcquiggd . To help plan, timescales for Polly#281 realistically in weeks, not days.

If we can sufficiently define the solution tho, we could up-for-grabs it (which could bring it forward). Or I could model it for one policy type, and we up-for-grabs the remainder ...

Have raised this up the priority tree, in view of the impact on Pollynator!

[mcquiggd]

@reisenberger

Clear... in that case I will implement Alternative Approach 3 from my message above, and implement two constructors this weekend, with attendant explanation, and a pre version tag on Visual Studio Market Place.

I might have time to assist with the changes to Polly...

David