3
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ The only "problem" I believe that can happen in this is if the waiterTask throws an exception it might not propagate properly. Though not entirely sure on that. \$\endgroup\$ – TheLethalCoder Apr 11 '17 at 15:13
6
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ The server would move a file and then the client would be told it was ready to pick up. However, when the client went to access the file it wasn't there yet, our best guess is that the files on the server hand't synced properly to the client. As the server isn't in our control we have to change the code to fix the issue. \$\endgroup\$ – TheLethalCoder Apr 11 '17 at 17:36
  • \$\begingroup\$ I agree with what you're saying except the DateTime stuff, I personally won't use that for timing anything. The cancellation token is from here as well so I assumed (maybe stupidly) that it should handle killing the task properly. \$\endgroup\$ – TheLethalCoder Apr 11 '17 at 17:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.