Wait for file method

I’ve been having problems with a client where the server would move a file on the server, the client would then try and access that file but get a FileNotFoundException. We believe this is due to the client not synchronising to the server properly.

To get around this I wrote the following method to wait for the file to arrive before it touches it, including a timeout:

private static async Task<bool> WaitForFile(string path, int timeout)
{
    CancellationTokenSource cts = new CancellationTokenSource();
    Task waiterTask = Task.Run(async() =>
    {
        while (!File.Exists(path))
        {
            await Task.Delay(10);
        }
    }, cts.Token);

    bool completed = await TimeoutAfter(waiterTask, timeout);
    if (!completed)
    {
        cts.Cancel();
    }

    return completed;
}

private static async Task<bool> TimeoutAfter(Task task, int timeout)
{
    CancellationTokenSource cts = new CancellationTokenSource();

    Task completedTask = await Task.WhenAny(task, Task.Delay(timeout, cts.Token));
    if (completedTask == task)
    {
        cts.Cancel();

        return true;
    }

    return false;
}

It’s not at all clear to me what the point of this code is: I assume it’s running on the server, but I can’t see how it would fit into the broader picture of client-server synch. It would probably be better to debug the underlying problem rather than try to hack a workaround.

That aside, the code has one important misunderstanding, one bad practice used twice, and is overcomplicated.

Misunderstanding

    Task waiterTask = Task.Run(async() =>
    {
        while (!File.Exists(path))
        {
            await Task.Delay(10);
        }
    }, cts.Token);

There’s no point passing in cts.Token because the task ignores it completely. (Task.Run would test whether the token has been cancelled before calling the action, but if the timeout is happening before the action is called then either the timeout is far too small or there’s a much bigger problem to fix with server load).

There isn’t actually a Task.Run(Action<CancellationToken>, CancellationToken>) method, so you can either forget CancellationTokenSource and use a different synchronisation method; or pull in either the token or cts directly from the outer scope.

Bad practice

CancellationTokenSource is IDisposable, but neither of the methods which create an instance are disposing it. Since its scope is clearly defined, the best practice would be a using statement.

Overcomplication

Two CancellationTokenSources just to handle a timeout for a single task, when you have full control over the task’s source? It seems to me that the KISS approach would be

private static async Task<bool> WaitForFile(string path, TimeSpan timeout)
{
    DateTimeOffset timeoutAt = DateTimeOffset.UtcNow + timeout;
    while (true)
    {
        if (File.Exists(path)) return true;
        if (DateTimeOffset.UtcNow >= timeoutAt) return false;
        await Task.Delay(10);
    }
}

If cancellation is important (it’s not in the original code, but it allows me to show how to use CancellationToken correctly, so…),

private static async Task<bool> WaitForFile(string path, TimeSpan timeout, CancellationToken cancellationToken = default(CancellationToken))
{
    DateTimeOffset timeoutAt = DateTimeOffset.UtcNow + timeout;
    while (true)
    {
        if (File.Exists(path)) return true;
        if (DateTimeOffset.UtcNow >= timeoutAt) return false;
        cancellationToken.ThrowIfCancellationRequested();
        await Task.Delay(10);
    }
}

(or, in this case)

private static async Task<bool> WaitForFile(string path, TimeSpan timeout, CancellationToken cancellationToken = default(CancellationToken))
{
    DateTimeOffset timeoutAt = DateTimeOffset.UtcNow + timeout;
    while (true)
    {
        if (File.Exists(path)) return true;
        if (DateTimeOffset.UtcNow >= timeoutAt) return false;
        await Task.Delay(10, cancellationToken);
    }
}