gebs gebs - 3 months ago 24
C# Question

Is this extension method thead safe?

I am having a hard time wrapping my head around extension methods. They are static methods inside static classes. How are they initialized internally? for example, I wrote the below extension method. Is it thread safe?

public static async Task TimeoutAfter(this Task task, TimeSpan timeSpan)
{
var cts = new CancellationTokenSource();
try
{
if (task.IsCompleted || timeSpan == Timeout.InfiniteTimeSpan)
return;
if (timeSpan == TimeSpan.Zero)
throw new TimeoutException();
if (task == await Task.WhenAny(task, Task.Delay(timeSpan, cts.Token)))
{
cts.Cancel();
await task;
}
else
{
throw new TimeoutException();
}
}
finally
{
cts.Dispose();
}
}

Answer

All extension methods do is turn

var result = myTask.TimeoutAfter(TimeSpan.FromSecconds(5));

in to

var result = ExtensionMethodClass.TimeoutAfter(myTask, TimeSpan.FromSecconds(5));

and nothing else. So whether or not a function is a extension method does not affect it's behavior at all, it is just a convince for programmers to not have to type out the long version from my above example.

As to if your code is thread safe, first you need to understand what "thread safe" means. I highly reccomend you read the article "What is this thing you call "thread safe"?" by Eric Lippert, it will greatly help you understand what thread safety means.

Your code does not access or mutate any external variables from it's scope, so the function itself is thread safe, but that does not mean it can't be used in "thread unsafe" ways. Fortunately you are lucky, both Task and TimeSpan guarantee thready safety in all of their methods and properties, so it is unlikely that you run in to any thread safety issues.


However all that being said, you do have a bug with a race condition. If task.IsCompleted returns true and task throws an exception you will never be notified of that exception. Also if timeSpan == Timeout.InfiniteTimeSpan your function will immedatly return with a completed task even though the passed in task is still running. You need to await task even if you are not planning on doing a timeout. Also, your try/finally can be simplifed to a using statement

public static async Task TimeoutAfter(this Task task, TimeSpan timeSpan)
{
    using(var cts = new CancellationTokenSource())
    {
        if (task.IsCompleted || timeSpan == Timeout.InfiniteTimeSpan)
        {
            await task;
            return;
        }
        if (timeSpan == TimeSpan.Zero)
            throw new TimeoutException();
        if (task == await Task.WhenAny(task, Task.Delay(timeSpan, cts.Token)))
        {
            cts.Cancel();
            await task;
        }
        else
        {
            throw new TimeoutException();
        }
    }
}

Lastly, if you have not done it yet, you will want to make a version that takes in Task<T> too in case you want to timeout tasks that return a result.

public static async Task<T> TimeoutAfter<T>(this Task<T> task, TimeSpan timeSpan)
{
    using(var cts = new CancellationTokenSource())
    {
        if (task.IsCompleted || timeSpan == Timeout.InfiniteTimeSpan)
        {
            return await task
        }
        if (timeSpan == TimeSpan.Zero)
            throw new TimeoutException();
        if (task == await Task.WhenAny(task, Task.Delay(timeSpan, cts.Token)))
        {
            cts.Cancel();
            return await task;
        }
        else
        {
            throw new TimeoutException();
        }
    }
}