Polly icon indicating copy to clipboard operation
Polly copied to clipboard

Extract static SystemClock to interface

Open eugbaranov opened this issue 6 years ago • 7 comments
trafficstars

The issue or feature being addressed

I'd like to propose extracting static SystemClock to an interface.

Details on the issue fix or feature implementation

These are initial commits with the end goal to eventually get rid of the ambient SystemClock instance by allowing specifying one when the policies gets created. This will allow to run all tests in parallel and will restrict rogue system clock injection during runtime.

Confirm the following

  • [x] I started this PR by branching from the head of the latest dev vX.Y branch, or I have rebased on the latest dev vX.Y branch, or I have merged the latest changes from the dev vX.Y branch
  • [x] I have targeted the PR to merge into the latest dev vX.Y branch as the base branch
  • [x] I have included unit tests for the issue/feature
  • [x] I have successfully run a local build

eugbaranov avatar Aug 21 '19 21:08 eugbaranov

@eugbaranov Thanks for this! We considered doing this couple years ago around #359 for the reasons you mention; great to have the reminder.

Any thoughts around syntax options for configuring the over-ridden clock on policies? I want to avoid new overloads of the already-over-proliferated, existing policy configuration overload format. I do plan to make a new syntax proposal which would make policy configuration syntax (generally) more elegant. Can imagine we might have choices for over-riding the policy clock like property-injection, extension method, or via the new policy builder syntax.

The usage here means we'd need to take a breaking-change on ITtlStrategy, so the transition to policy-scoped clocks would probably be part of a major release.

Linking also: #156

reisenberger avatar Sep 01 '19 10:09 reisenberger

Avoiding new overloads, I think the easiest would be to have a new method which you could use to set non-standard system clock, e.g. Policy.WithSystemClock(new TestSystemClock()).Timeout(30).

eugbaranov avatar Sep 02 '19 13:09 eugbaranov

Is this using ISystemClock by chance?

jjxtra avatar Jan 14 '20 03:01 jjxtra

Apologies for not updating this PR sooner - I was trying to implement new WithSystemClock syntax but it turned out to be trickier than I've anticipated.

eugbaranov avatar Jan 15 '20 00:01 eugbaranov

@jjxtra Polly is not using any ISystemClock from the base class libraries (was that the question?). Polly has a SystemClock class which aggregates a few time-related helpers. EDIT: We may split these out also if we move to an injectible form.

@eugbaranov No problem. It may be even easier to switch to an injectible clock (with nice syntax) when the syntax changes proposed here are enacted. (Cannot confirm at this stage, but work on this may happen as part of v8 this spring).

reisenberger avatar Jan 15 '20 08:01 reisenberger

@reisenberger the proposed syntax looks like a good improvement however since I'm adding clock to the PolicyBuilder I found policies not using that to be the biggest challenge, e.g. Policy.Cache - at the moment is returns CachePolicy.

Ideally I wouldn't like to use something like Policy.WithClock(testClock).Cache(...) but I suppose Policy.Cache(builder => builder.WithClock(testClock).CacheProvider(...)).

eugbaranov avatar Jan 15 '20 10:01 eugbaranov

@eugbaranov Yes, exactly - that's exactly the idea.

reisenberger avatar Jan 16 '20 08:01 reisenberger

Closing due to inactivity.

Please open a new pull request if you would like to continue with a proposal for this change which we could consider for a v8 of Polly.

martincostello avatar Dec 16 '22 15:12 martincostello