theraot / Theraot

Backporting .NET and more: LINQ expressions in .net 2.0 - nuget Theraot.Core available.
MIT License
160 stars 30 forks source link

Task.Wait doesn't propagate exception #132

Closed NN--- closed 4 years ago

NN--- commented 4 years ago
using System;
using System.Threading;
using System.Threading.Tasks;

namespace ConsoleApp14
{
    class Program
    {
        static void Main(string[] args)
        {
            try
            {
                var cts = new CancellationTokenSource();
                var task = Task.Run(
                    async () =>
                    {

                            await Task.Delay(TimeSpan.FromSeconds(10), cts.Token);

                    },
                    cts.Token);
                cts.CancelAfter(TimeSpan.FromSeconds(1));
                task.Wait();

                // .NET 2.0 - 3.5 reach this line
                Console.WriteLine("Must not get there");
            }
            catch (AggregateException)
            {
                Console.WriteLine("Expected");
            }
        }
    }
}

I guess related to #120

theraot commented 4 years ago

This happens: The Task gets marked cancelled, but the CancellationToken on the Task says that no cancellation was requested. Why? Because the task has a CancellationToken from the default source. Why? Because Task.Run uses Unwrap and Unwrap does not copy the CancellationToken. I'll fix that.

NN--- commented 4 years ago

I tried to find out the reason but didn’t succeed.

theraot commented 4 years ago

Simple test shows that a Task should throw even without a CancellationToken with cancellation requested (Tried in LinqPad):

TaskCompletionSource<int> tcs1 = new TaskCompletionSource<int>();
Task<int> t1 = tcs1.Task;
tcs1.TrySetCanceled();
t1.Wait();

So, I think I'll make it throw when canceled even if the CancellationToken.CancellationToken does not throw.

I'll see if Unwrap should or should not copy the CancellationToken. This could be two bugs instead of one.

theraot commented 4 years ago

Trying the code above in .NET 2.0 throws an AggregateException with TaskCanceledException with a CancellationToken with IsCancellationRequested = true. While the same code in LinqPad has IsCancellationRequested = false.

I can also confirm that Unwrap should copy the CancellationToken.

Edit: that is a total of 3 things to fix:

theraot commented 4 years ago

Ok, this is the problem…

If TrySetCanceled does not set a CancellationToken, this line hangs: https://github.com/theraot/Theraot/blob/master/Framework.Core/System/Threading/Tasks/Task.cs#L738

And then #120 comes back.

I haven't found any nice workaround.

--

Edit: To be clear, the changes that I will fix the original issue. The line Console.WriteLine("Must not get there"); won't be reached. It is just that TrySetCanceled will always result in a TaskCanceledException with a cancelled token, while that is not true in .NET framework.

NN--- commented 4 years ago

Is there an updated NuGet ?

theraot commented 4 years ago

Just submitted. NuGet 3.2.0.

NN--- commented 4 years ago

Thanks a lot !

NN--- commented 4 years ago

Works well.