Async sleep + continue on captured ctx. Fixes #49

[yevhen]

No description provided.

[yevhen]

@michael-wolfenden any updates on this?

[reisenberger]

@yevhen Looking at your PR, I see (only now...) that you have also tackled async sleep as await Task.Delay as part of this PR on captured ctx, same as I have raised in #56. I have done it slightly differently, but same underlying principle. Will be happy with whatever Polly community want to integrate. Think this is an important fix.

[yevhen]

@reisenberger ye, think the same!

[reisenberger]

@yevhen This overload RetryAsync(this PolicyBuilder policyBuilder, bool continueOnCapturedContext) doesn't make use of the supplied continueOnCapturedContext value. Does it want a small correction to:
return policyBuilder.RetryAsync(1, continueOnCapturedContext); ?

(Apologies @yevhen @joelhulen for intervening but hope useful - I spotted this while doing some exploratory investigation of how to add cancellation support!)

[joelhulen]

@reisenberger No apology needed! It takes volunteers like you to help spot these things and keep progress marching forward. All help is gladly accepted :)

[yevhen]

@joelhulen +1 @reisenberger nice catch! I was trying really hard to not introduce breaking changes and got lost with all those pesky overloads. Many thanks and you don't need to apologize for that. It's great to have another pair of eyes! 👀

[reisenberger]

@yevhen @joelhulen On the subject of all those overloads (agree), it might be worth some forethought - if we are going to introduce both bool continueOnCapturedContext, and CancellationToken cancellationToken (ref #46) - on what priority order we want the parameters to run in.

In the retry overloads in this #53, @yevhen @joelhulen (see snip below) ...

... I like the order in RetryAsync(...) and RetryForeverAsync(...) (it seems logical that bool continueOnCapturedContext comes after how many retries but before Action onRetry ... ). But, for WaitAndRetryAsync(...) I would suggest bool continueOnCapturedContext should come after Func<int, TimeSpan> sleepDurationProvider or IEnumerable<TimeSpan> sleepDurations. The how-long-to-delay-for is core to expressing WaitAndRetryAsync(...) and should come right near the (. This would also place overload parameters in a consistent order for all retry variants:

how many times / with what delay to retry,
whether to continue on captured context,
action on retry

If/when introducing CancellationToken could then be expanded to:

how many times / with what delay to retry,
cancellation token,
whether to continue on captured context,
action on retry

(or cancellation token / captured context in the other order)

[yevhen]

Very good suggestion. I will update PR.

re order of CancellationToken/ContinueOnCapturedContext I think any order is fine.

The more important is to figure out what to do with all those overloads? If we introduce another set of overloads (permutation with CancellationToken) we may end up with unmaintainable mess. I think that number of parameters is already far behind the number of parameters acceptable for Clean Code 😜

What if we introduce something like Parameter Object? Alternatively we can try to employ automation, say T4 template which will auto-generate overloads with all that currently copy-n-pasted xml documentation.

[reisenberger]

@yevhen Thanks for this great contribution; we're looking to get this merged soon.

Re overloads I can see three options, not sure if any quite come together (tho if anyone can see a bright way through this conundrum, please shout)

Optional parameters: Would be ideal (my preferred) if it weren't for the caller/call-ee problem in how this is implemented in C#. IE if we later have to change signature, can cause run-time failure if people just drop in new DLL rather than recompile against. Believe why most of .NET framework doesn't use optional params.

T4: Slight concern about introducing something lesser-known which may raise the barrier of entry for other contributors. Also, does this just help with documentation (eg xml comments)?

Parameter object: Problem I can see is there's already the optional onRetry parameter. If we move that into a parameter object, it'd be a breaking change 😢. If we don't move it in to a parameter object, some of the optional parameters will be in a parameter object and some not 😞 .

Also, .RetryAsync(int, new RetryAsyncOptions(CancellationToken, bool), Action) not necessarily more concise when calling than .RetryAsync(int, CancellationToken, bool, Action)
.
Open to views, but it may be the overloads are just something we're going to have to live with, rather like Task.Factory.StartNew() in the framework

[reisenberger]

@yevhen Thanks again for this important PR! Unless anyone has brighter idea on overloads, team AppvNext are ready to accept/merge.

Are you able to update the PR with the following minor adjustments? - then it will be good to go!

• resolve conflicts by rebasing against latest master
• fix overload not using bool continueOnCapturedContext mentioned few days ago
• adjust bool continueOnCapturedContext parameter position per discussion few days ago and a few more line notes I will add
• update changelog.MD to describe change (but not GitVersionConfig.yaml; @joelhulen will update yaml when pushing to Nuget)

Thank you!

[reisenberger]

Request move bool continueOnCapturedContext to be last parameter, in ImplementationAsync

[yevhen]

If you're ready to publish new Nuget package then I'm ready to update this
in few days )))

2015-12-10 1:26 GMT+02:00 reisenberger notifications@github.com:

@yevhen https://github.com/yevhen Thanks again for this important PR!
Unless anyone has brighter idea on overloads, team AppvNext are ready
to accept/merge.

Are you able to update the PR with the following minor adjustments? - then
it will probably be good to go!

• resolve conflicts by rebasing against latest master
• fix overload not using bool continueOnCapturedContext mentioned few
  days ago
• adjust bool continueOnCapturedContext parameter position per
  discussion few days ago and a few more line notes I will add
• update changelog.MD to describe change (but not
  GitVersionConfig.yaml; @joelhulen https://github.com/joelhulen will
  when pushing to Nuget)

Thank you!

—
Reply to this email directly or view it on GitHub
#53 (comment).

[reisenberger]

Request add overload, not optional parameter, because of optional-params-compiled-at-callee-not-caller problem in c# implementation of optional parameters

[joelhulen]

@yevhen Once I merge your PR (after your changes), then I'll test locally and publish the new NuGet package once verified.

[reisenberger]

Per previous discussion, move bool continueOnCapturedContext to after sleepDurations

[reisenberger]

( @yevhen / all, apologies, and thanks, for bearing with flurry of line notes ) ( as you rightly say, yevhen, large number of overloads ...;)

But purpose: If we fix parameter order appropriately now, the cancellation support #46 can be built right around this without any breaking changes to the API between releases

[joelhulen]

@yevhen: We're ready to include your PR into version 3.0! This along with cancellation features @nedstoyanov and @reisenberger are working on will really help make Polly even more mature. Thank you so much for helping us push the library forward.

If you are ready, please fix the parameter order, as outlined in detail by @reisenberger. Once that's done, we can merge it in.

[yevhen]

Sorry, guys. I'll definitely do this till EOW. At the moment I'm incredibly busy, preparing for 2 public presentations this week.

[joelhulen]

Thanks, @yevhen! I hope you know I wasn't trying to pressure you. It's a crazy a time of year for all of us.

[yevhen]

No worries ))

2015-12-16 21:18 GMT+02:00 Joel Hulen notifications@github.com:

Thanks, @yevhen https://github.com/yevhen! I hope you know I wasn't
trying to pressure you. It's a crazy a time of year for all of us.

—
Reply to this email directly or view it on GitHub
#53 (comment).

[joelhulen]

@yevhen: Hope you had a great holiday weekend :) Just wondering if you've had a chance to fix up those last few things? We're hoping to release a new version with your changes by year's end, if possible.

Also, just so you're aware, you'll need to rebase your local repo with the latest changes we incorporated yesterday. Thanks!

[reisenberger]

@yevhen @joelhulen Super: it was my absolute assumption that we would want to be completely deterministic about this across the library, but wanted to involve you guys.

@yevhen : those two:

if (!(await policyState.CanRetryAsync(ex)))
/
await SystemClock.SleepAsync(currentTimeSpan);

... appear to the only two not currently controlling the synchonization context (.ConfigureAwait()) via the new bool continueOnCapturedContext.

[yevhen]

guys, what do you have set for Git's core.autocrlf and what version of git do you have installed? I'm observing absolutely weird behavior with files being modified just after the clone, despite having core.autocrlf=false. I'm using Git v1.9.5.

[reisenberger]

I've seen cr/lf oddness too but don't know the setting - over to @joelhulen ? - would be good to iron this out.

[reisenberger]

Apologies - more thoughts coming on this:

Because of the way the codebase has evolved (see end of post), we now have / are working towards a number of constructs in the codebase like / effectively equivalent to: [+]

if (continueOnCapturedContext)
   await action();
else
   await action().NotOnCapturedContext();

or

if (onCapturedContext)
   return await task;
else
   return await task.NotOnCapturedContext();

Can these not just be shortened to await task.ConfigureAwait(continueOnCapturedContext) ?

(It could also have the advantage of explicitly configuring the await either way - it probably makes no practical difference in the current .NET implementation, but may be clearer to subsequent code readers what is going on.)

The history of this in the Polly library was: (from memory- could be wrong on some detail)

• Async implementation was initially added, without (I think; cld be wrong) code controlling continuation context, ie plain await Task
• Contributors then logically requested the addition of .ConfigureAwait(false), as is standard for library code, particularly if library code shouldn't care about sycnhronisation context. This was abbreviated to nice extension methods .NotOnCapturedContext(this Task task) etc.
• But since the point of Polly is to run delegates (wrapped in some way), we might sometimes need to control the synchonisation context the delegates run on - hence yevhen's contribution. We can see why, built on the existing .NotOnCapturedContext(), this leads to patterns like [+] above, but I'm wondering if we can now shorten to the much more direct await task.ConfigureAwait(continueOnCapturedContext); ?

(with apologies again for late additional realisation ... what do you guys think?)

[yevhen]

@reisenberger ye, NotOnCapturedContext() doesn't make much sense anymore. I'll incorporate this realization ;)

[yevhen]

I do believe problems with line endings are due to use of .gitattributes file. I observed similar problems on my projects when this file was included. It needs to be deleted from source. It only creates more problems.

[yevhen]

I've added all change requests as separate commits so it should be easy to review. If you want I can squash everything into single commit.

[yevhen]

Hey, build failed for some weird reasons. Please kick a rebuild.

[joelhulen]

@yevhen there were some build failures yesterday due to connectivity issues between appveyor and NuGet. I'll kick off another build.

[joelhulen]

Build succeeded :)

[yevhen]

Awesome! =)

[yevhen]

Ah, I forgot to modify CHANGELOG. Tomorrow then.

Can you do it? ))

[joelhulen]

I can modify the CHANGELOG if you haven't already.

[joelhulen]

@reisenberger @yevhen: I just posted instructions on fixing the line endings to our wiki site. See if these steps help. If not, feel free to update the page :)

[reisenberger]

@yevhen Thanks for your exemplary clear presentation! (a standard for us to follow 😄 ). Have been a good second pair of 👀 and @joelhulen looks good to go to me! Many thanks yevhen for tracking all that detail...

[yevhen]

That would be great, cause I'm out of city till 10th of January 😄

30 дек. 2015 г., в 19:12, Joel Hulen notifications@github.com написал(а):

I can modify the CHANGELOG if you haven't already.

—
Reply to this email directly or view it on GitHub.

[yevhen]

Thanks guys! It was a pleasure working with you on this PR. Keep it up!

30 дек. 2015 г., в 21:15, reisenberger notifications@github.com написал(а):

@yevhen Thanks for your exemplary clear presentation! (a standard for us to follow ). Have been a good second pair of and @joelhulen looks good to go to me! Many thanks yevhen for tracking all that detail...

—
Reply to this email directly or view it on GitHub.

[reisenberger]

Thanks @yevhen . Cancellation support (#46) coming shortly (I've been working on it in background), which should round out the async support in Polly nicely. 😉

[yevhen]

Awesome! That would be great addition!

30 дек. 2015 г., в 21:35, reisenberger notifications@github.com написал(а):

Thanks @yevhen . Cancellation support (#46) coming shortly (I've been working on it in background), which should round out the async support in Polly nicely.

—
Reply to this email directly or view it on GitHub.