Polly: Conditional Timeout using timeoutProvider (no timeout, via infinite timeout)

[Danation]

Right now, using a TimeoutPolicy assumes that there should always be a timeout. But when specifying a timeoutProvider function, it would be nice to be able to choose not to timeout at all in certain circumstances.

A simple workaround is to specify a very long timeout (many days), but it would be better to have a more clear option. Perhaps by making it nullable or marking a particular constant as "no timeout" such as TimeSpan.Zero.

What do you think?

[Danation]

Hunting through the code, it looks like it uses CancellationTokenSource.CancelAfter(), which can take a -1. I wasn't able to dig deep enough into the source to find how it handles -1, but after some testing, it looks like it does what I'd hope (no cancellation). So it looks like returning TimeSpan.FromMilliseconds(-1) in a timeoutProvider works. Could anybody confirm that I'm correct on that?

[reisenberger]

Hi @danation. The Policy.Timeout(...) configuration overloads will currently throw ArgumentOutOfRangeException on constant values <= TimeSpan.Zero, but you can use the Func<TimeSpan> timeoutProvider to return these values. My reading of CancellationTokenSource.CancelAfter() and Timer source code is that it will behave as you desire.

---

We'd accept a PR to change the guard clauses in TimeoutSyntax and similarly TimeoutSyntaxAsync, to formalise allowing Timeout.InfiniteTimeSpan. Thanks for raising this!

[urig]

I'd like to "grab" this. Would you say fixing the guard clauses and related unit tests would suffice? In other words, how would I go about verifying the correct behavior for the Timeout when set to TimeSpan.Zero?

[reisenberger]

@urig Cool! Thank you for offering to pick this up!

The const that Microsoft define as Timeout.InfiniteTimeSpan isn't (you'll see from the link) the same as TimeSpan.Zero. Though the first post here suggested TimeSpan.Zero to represent infinite, I think we should stick to what Microsoft use.

Ideally we would change Polly's guard clauses to permit two new cases:

• Timeout.InfiniteTimeSpan
• TimeSpan.Zero (which we would expect to time out any operation more-or-less immediately).

Re testing:

Timeout.InfiniteTimeSpan: I agree with your comment "fixing the guard clauses and related unit tests would suffice" (we only go as far as testing that configuring Timeout.InfiniteTimeSpan is possible). It's hard and too time-consuming :grin: to have a unit test that tests an infinitely-long operation never times out, right?

TimeSpan.Zero: We should test it is possible to configure. It would be good also to add a unit test that, when configured with TimeSpan.Zero, it does time out a fairly short operation. There are very similar unit tests already in the test classes, in the sections #region Timeout operation - pessimistic and #region Timeout operation - optimistic .

Do you think this is a good plan? Do you want to take on both cases or only one?

Thanks again for coming forward for this!

[urig]

@reisenberger many many thanks for the detailed help. I'd like to take it slow and start with one of the features, let's say TimeSpan.InfiniteTimeSpan. Perhaps we can open another issue for TimeSpan.Zero and I can pick that up right after?

[reisenberger]

@urig Sounds good to me!

I reflected again on TimeSpan.Zero overnight, and was like: what would be the point in configuring a TimeoutPolicy with TimeSpan.Zero? My reasoning yesterday had been that _conceptually_, somebody might have a dynamically calculating timespan which happened to calculate at zero (eg perhaps other actions elsewhere dictate we can no longer afford to wait for the action we're about to execute), so we should allow zero. But if they have a calculated timespan, they should/would likely be using the creation overloads of TimeoutPolicy which take a Func<TimeSpan>, not the creation overloads which take a const/fixed timespan. What would be the point of configuring and using a TimeoutPolicy with a _fixed_ TimeSpan.Zero? On reflection think we can forget the TimeSpan.Zero case - at least until someone can demonstrate a good reason for it :grin:

[urig]

@reisenberger Ok if I submit a PR just for the sync part (w/o Async for now)? I'd like to show you my work and get feedback.

[reisenberger]

@urig Sure! :+1:

[urig]

Thank you. PR https://github.com/App-vNext/Polly/pull/424 is ready. Your feedback most welcome :)

[reisenberger]

Closed via #424

[reisenberger]

@urig @Danation This feature has now been released as part of Polly v5.9.0!

Big thanks to @urig for the contribution! :100: Credit has been added to you, @urig, in the acknowledgements in the readme :+1:

[urig]

Wonderful news. Huge thanks to you @reisenberger for your help in working on this! May all OSS maintainers be as kind and patient as you are :)