Timeout not working as expected [EDIT: Async pessimistic timeout does not timeout synchronous delegates]

[udlose]

Please clarify if I am misunderstanding how the Timeout Policy works. I would expect this to cause a TimeoutRejectedException but it does not. I have a Task.Delay(4000).Wait() defined in the func that Polly executes. I would think that anything in that Func that takes longer than the specified Timeout would throw TimeoutRejectedException but this is not the behavior I'm seeing.

Taken from Polly samples

            // Define our timeout policy: time out after 2 seconds.  We will use the pessimistic timeout strategy, which forces a timeout - even when the underlying delegate doesn't support it.
            var timeoutPolicy = Policy
                .TimeoutAsync(
                    TimeSpan.FromSeconds(2),
                    TimeoutStrategy.Pessimistic,
                    onTimeoutAsync: (callContext, span, task) =>
                    {
                        return Task.CompletedTask;
                    }
                );

            // Define our waitAndRetry policy: keep retrying with 4 second gaps.  This is (intentionally) too long: to demonstrate that the timeout policy will time out on this before waiting for the retry.
            var waitAndRetryPolicy = Policy
                .Handle<Exception>() // Exception filtering!  We don't retry if the inner circuit-breaker judges the underlying system is out of commission!
                .WaitAndRetryForeverAsync(
//                    attempt => TimeSpan.FromSeconds(4),
                      attempt => TimeSpan.FromMilliseconds(100),
                  (exception, calculatedWaitDuration) =>
                    {
                    });

            FallbackPolicy<String> fallbackForAnyException = Policy<String>
                .Handle<Exception>()
                .FallbackAsync(
                    fallbackAction: /* Demonstrates fallback action/func syntax */ async ct =>
                    {
                    },
                    onFallbackAsync: async e =>
                    {
                    }
                );

            PolicyWrap<String> policyWrap = fallbackForAnyException.WrapAsync(timeoutPolicy).WrapAsync(waitAndRetryPolicy);

            while (!Console.KeyAvailable && !cancellationToken.IsCancellationRequested)
            {
                try
                {
                    Func<CancellationToken, Task<string>> foo = (ct) =>
                    {
                        //shouldn't this cause a TimeoutRejectedException???
                        Task.Delay(4000).Wait();  
                        return Task.FromResult("hello");
                    };

                    string msg = await policyWrap.ExecuteAsync(foo, cancellationToken);
                }
                catch (Exception e)
                {
                }

                // Wait half second
//                await Task.Delay(TimeSpan.FromSeconds(0.5), cancellationToken);
            }

[reisenberger]

Hey @udlose. TL;DR You happen to have hit upon an async test delegate which isn't truly async. The pessimistic async timeout policy, by design, chose not to handle sync-not-async delegates in the async timeout policy, because to do so would have meant a performance penalty for the majority async case.

---

Explanation: why does the posted test code not work?

The Polly async TimeoutPolicy with TimeoutStrategy.Pessimistic intentionally isn't catering for the edge case of async delegates which are not 'truly async', but are in fact actually synchronous.

By 'truly async' I mean: conform to the async method pattern of returning to the caller a Task instance which represents the on-going asynchronous execution of the method. The pessimistic async timeout implementation relies on that normative pattern: it needs to obtain this Task, in order to run a timing task in parallel, and see which completes first.

Returning that Task - the one which represents the on-going asynchronous execution of the method - happens automatically when an async method hits its first internal await statement. Until it hits its first internal await, however, every await-ed method or delegate in fact runs synchronously on the calling thread. It happens that this test delegate:

// [1]
ct => {
    Task.Delay(4000).Wait();  //shouldn't this cause a TimeoutRejectedException???
    return Task.FromResult("hello");
}

contains no await statement, and so runs purely synchronously, right through to return. It doesn't return any Task until it has already completed (including the four-second delay). So the calling timeout policy never regains control until the test delegate has completed (including the four-second delay). So the calling timeout policy can never time it out.

Change the delegate to the following, and it will work:

// [2]
async ct => {
    await Task.Delay(4000);
    return "hello";
}

In this version, as soon as await Task.Delay(4000) is hit, a Task is returned to the calling policy representing the on-going asynchronous execution of the delegate. The timeout policy implementation then resumes control, and can determine if the executed delegate or timing task completes first; and can time out on (walk away from waiting for) the test delegate, when the timeout elapses.

[reisenberger]

Follow-up discussion: Should Polly handle the case [1]?

Could Polly async TimeoutPolicy with TimeoutStrategy.Pessimistic be coded to handle the purely-synchronous delegate case [1] above? And if so, should it?

It could: it could adopt exactly the same strategy as sync pessimistic timeout policy, of spinning off the executed delegate as a separate Task with Task.Run().

Should Polly do this? At design time, I elected not. For the vast majority of async cases, well-conforming to the async method pattern, that Task.Run() would represent a performance hit, an extra allocation and context/scheduling switch which are completely unnecessary to the vast majority, well-formed async case. Handling the non-conforming edge case at the expense of majority performance was declined.

Could this burn Polly users in production scenarios? It could, but only if they are faced with executing these 'faux'-asynchronous (in fact purely-synchronous) delegates through async, pessimistic TimeoutPolicy.

The main scenario I have envisaged where this might occur could be if some third-party component implements faux async-over-sync wrappers for long-running delegates, eg FooAsync() => Task.FromResult(Foo()) for some long-running Foo(). A Polly user might expect asynchronous pessimistic timeout policy to work on these, when it won't. However, it is well documented that such wrappers are bad practice. And, Polly users in any case have the workround of recourse to the synchronous pessimistic TimeoutPolicy for these cases.

---

I've followed this up at blog-post length because I appreciate this behaviour (async pessimistic timeout not handling these) could cause confusion, given the stated 'walk away from anything' power of pessimistic timeout.

I'd be interested if community members can think of any real-world async delegate cases, for which this design decision might cause difficulty.

EDIT: Updated the TimeoutPolicy wiki to document this.

[udlose]

Thank you for the thorough explanation! I assume in the TimeoutStrategy.Optimistic, it is completely up to the calling code to catch the timeout since, as documented, it assumes cooperative cancellation? If so, how does the calling code "catch" the timeout?

[reisenberger]

When TimeoutPolicy in either optimistic or pessimistic mode times out on an executed delegate, a TimeoutRejectedException is thrown. That exception can be caught either by the user code that executed through TimeoutPolicy, or by another Polly policy - for example, a RetryPolicy, or a FallbackPolicy handling TimeoutRejectedException, placed further out in the PolicyWrap.

Does this cover the sense of 'catch' the timeout in your question?

[reisenberger]

@udlose Let us know (after Thanksgiving!) if we can help any further on this. Re your question:

how does the calling code "catch" the timeout? [in TimeoutStrategy.Optimistic]

... there could be two senses.

(1) Per previous comment, a TimeoutRejectedException is thrown back onto the code placing the call through TimeoutPolicy.
(2) If the question is what happened to the task being executed (which was timed out...) : in TimeoutStrategy.Optimistic, that task should honour the CancellationToken and thus cancel itself (it won't continue running). If it wants to pass information back to the calling code about its state at the time of cancellation, that could be done by throwing a custom OperationCanceledException containing extra information. That exception is already passed back to calling code as the inner exception of the TimeoutRejectedException. If we extend Polly per #338, that exception would also be passed to onTimeout/Async, which might be a better place eg to capture info for logging.

[mrmartan]

@reisenberger

await Policy.TimeoutAsync(TimeSpan.FromMilliseconds(500)).ExecuteAsync(async () =>
{
    await Task.Delay(4000);
    return await this.TryGetRedis<TResult>(cacheKey);
});

[16:22:07 INF] Executing action method X - Validation state: Valid
[16:22:11 INF] Executed action method X, returned result Microsoft.AspNetCore.Mvc.OkObjectResult in 4096.9311ms.

I am just not able to hit the timeout. I've tried various methods. The snippet above is the gist of it.

[reisenberger]

@mrmartan See the Polly Timeout documentation. TimeoutPolicy with TimeoutStrategy.Optimistic depends on co-operative cancellation. Try something like:

await Policy.TimeoutAsync(TimeSpan.FromMilliseconds(500)).ExecuteAsync(async ct =>
{
    await Task.Delay(4000, ct);
    return await this.TryGetRedis<TResult>(cacheKey);
}, CancellationToken.None // CancellationToken.None here indicates you have no independent cancellation control you wish to add to the cancellation provided by TimeoutPolicy.
);

If you need to be able to walk-away from an execution that doesn't honour CancellationToken, use TimeoutStrategy.Pessimistic

[mrmartan]

Sry, my bad. Missed the overload.

Although my example code is arguably what you would expect to work just from looking at it.

[reisenberger]

Np. Re:

my example code is arguably what you would expect to work just from looking at it

Agreed. There was a trade-off at design time. TimeoutStrategy.Pessimistic works for your sample code, but it is more expensive in terms of resource consumption (especially for the sync case). We made TimeoutStrategy.Optimistic the default case, since it seemed good practice to encourage developers towards co-operative cancellation by CancellationToken (which consumes less resource), rather than making the sledge-hammer version (TimeoutStrategy.Pessimistic) the default - which also has (for users to grok) complications about how to catch errors from the walked-away-from Tasks. (Edit: in case this seems like magic information to anyone coming later to the thread, it is all covered in more detail in the doco.)

That's just to explain the rationale of the decision (why your case doesn't work as the default), but I agree: it makes for this speedbump on learning how to use the policy.