awaiting task with timeout

Question

I'm trying to write a helper method which allows me to pass in an arbitrary task and a timeout. If the task completes before the timeout, a success delegate is called, otherwise an error delegate is called. The method looks like this:

    public static async Task AwaitWithTimeout(Task task, int timeout, Action success, Action error)
    {
        if (await Task.WhenAny(task, Task.Delay(timeout)) == task)
        {
            if (success != null)
            {
                success();
            }
        }
        else
        {
            if (error != null)
            {
                error();
            }
        }
    }

Now this seems to work most of the time, but i wanted to write some tests as well to make sure. This test, to my surprise fails, and calls the error delegate instead of the success:

        var taskToAwait = Task.Delay(1);
        var successCalled = false;

        await TaskHelper.AwaitWithTimeout(taskToAwait, 10, () => successCalled = true, null);

        Assert.IsTrue(successCalled);

This test, however, is green:

        var taskToAwait = Task.Run(async () =>
        {
            await Task.Delay(1);
        });

        var successCalled = false;

        await TaskHelper.AwaitWithTimeout(taskToAwait, 10, () => successCalled = true, null);

        Assert.IsTrue(successCalled);

How do i make both tests green? Is my usage of Task.WhenAny incorrect?

Solution

Timers are inaccurate. By default their accuracy is around 15 ms. Anything lower than that will trigger in 15ms interval. Refer related answer.

Given that you have 1ms timer and 10ms timer; both are roughly equal so you get inconsistent results there.

The code you wrapped in Task.Run and claims to be working is just a coincidence. When I tried several times, results are inconsistent. It fails sometimes for the same reason mentioned.

You're better off increasing the timeout or just pass in a already completed task.

For example following test should consistently pass. Remember that your test should be consistent not brittle.

[Test]
public async Task AwaitWithTimeout_Calls_SuccessDelegate_On_Success()
{
    var taskToAwait = Task.FromResult(0);

    var successCalled = false;

    await TaskHelper.AwaitWithTimeout(taskToAwait, 10, () => successCalled = true, ()=>{ });

    Assert.IsTrue(successCalled);
}

For never ending task use TaskCompletionSource and don't set its result.

[Test]
public async Task AwaitWithTimeout_Calls_ErrorDelegate_On_NeverEndingTask()
{
    var taskToAwait = new TaskCompletionSource<object>().Task;

    var errorCalled = false;

    await TaskHelper.AwaitWithTimeout(taskToAwait, 10, () => { }, ()=> errorCalled = true);

    Assert.IsTrue(errorCalled);
}

Also I recommend you to avoid using null. You can just pass the empty delegate as a parameter. Then you don't want to have null checks scattered all over your codebase.

I'd write the helper method as:

public static async Task AwaitWithTimeout(this Task task, int timeout, Action success, Action error)
{
    if (await Task.WhenAny(task, Task.Delay(timeout)) == task)
    {
        success();
    }
    else
    {
        error();
    }
}

Note that above method is an extension method; so you can call it with task instance.

await taskToAwait.AwaitWithTimeout(10, () => { }, ()=> errorCalled = true);//No nulls, just empty delegate

Comments

[user1202032]

Thank you. Do you have any suggestions as how i should test the error scenario(s)? That is, input a task that never completes. For now, im still using Task.Delay() for this (just setting the delay very high, and the timeout somewhat low)

[Sriram Sakthivel]

@user1202032 Sure. Updated my answer to address never ending task.

[Kirill Shlenskiy]

While the core concept (using Task.WhenAny to await on two tasks) is spot-on, the API around it feels dirty. Action for success is messy if you need to wire up another Task from it - you'd end up with ugly captures. I'd simply throw a TimeoutException in the timeout scenario and treat successful completion as, well, successful completion. In performance-critical cases a Task<bool> return type would also work (where true means successful completion and false means timeout).

[Kirill Shlenskiy]

Then there's also the handling of the eventual completion of the timed-out Task (discussed here: blogs.msdn.com/b/pfxteam/archive/2012/10/05/…). And, of course, don't forget ConfigureAwait(false) - there's absolutely no reason not to have it in helper or library methods.

[Sriram Sakthivel]

@KirillShlenskiy Yes exactly my thoughts. Task<bool> makes more sense than delegates. Even better TimeoutExceptiion. I leave it to OP.