Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How can discard exception in async task correctly ? #92

Open
shikihane opened this issue Nov 23, 2021 · 2 comments
Open

How can discard exception in async task correctly ? #92

shikihane opened this issue Nov 23, 2021 · 2 comments

Comments

@shikihane
Copy link

shikihane commented Nov 23, 2021

I want to simplify the process of cancel async task with MethodBoundaryAspect.
So, I write the code below:

    public class CreateAysnContext : OnMethodBoundaryAspect
    {
        public override void OnEntry(MethodExecutionArgs args)
        {
            Cancelable.cts = new();
        }
    }

    public class CatchCancel : OnMethodBoundaryAspect
    {
        public override void OnException(MethodExecutionArgs args)
        {
            if (args.Exception is OperationCanceledException)
            {
                // Error : An exception will be throw in soon after:  System.InvalidOperationException:“An attempt was made to transition a task to a final state when it had already completed.”
                args.FlowBehavior = FlowBehavior.Continue;

            }
        }
    }

    public class Cancelable : OnMethodBoundaryAspect
    {
        public static CancellationTokenSource cts;
        public static void Term()
        {
            cts.Cancel();
        }
        public override void OnEntry(MethodExecutionArgs args)
        {
            cts.Token.ThrowIfCancellationRequested();
        }
        public override void OnExit(MethodExecutionArgs args)
        {
            cts.Token.ThrowIfCancellationRequested();
        }
    }

and caller:

 public class TestViewModel : Screen
  {
      [Cancelable]
      public async Task LongTask(int i)
      {
          Trace.WriteLine(i);
          await Task.Delay(2000);
      }
  
      [CreateAysnContext]
      [CatchCancel]
      public async Task Run()
      {
          // simulate long task
          for (int i = 0; i < 1000; i++)
              await LongTask(i);
      }
  
  
      public void Stop()
      {
          Log.Information("Task will be cancel");
          Cancelable.Cancel();
      }
  }

But he will throw a System.InvalidOperationException:“An attempt was made to transition a task to a final state when it had already completed.”
How can i discard OperationCanceledException correctly ?
The effect I expected is as below:

  public async task ExpectFun()
  {
      var token = new CancellationToken();
      try
      {
          while (true)
          {
              token.ThrowIfCancellationRequested( );
              await DoSomeAsyncThing( );
          }
      }
      catch (OperationCanceledException)
      {
          // discard  OperationCanceledException
      }
  }
@Ralf1108
Copy link
Collaborator

The "InvalidOperationException" indicates there is a bug in the FlowBehavior-Handling for async methods. Async support was added by @keith-anders. This would have to be investigated.

But can you explain the original use case as I think the usage of aspect here is not recommended?
I don't understand what the expectation should be if every method which is annotated with "CreateAysnContext" overwrites the current "Cancelable.cts". Why not create the "CancellationTokenSource" in the TestViewModel and provide it to the task itself.. then the task could check the "CancellationTokenSource" by itself and also support preemptive stop the execution.

Also, using static variables from other classes in aspect looks like a code smell. If you apply your aspect multiple times to other methods you will just create a mess of dangling "CancellationTokenSources". Which CancellationTokenSource will then be canceled? Think about if you have multiple "TestViewModels". And creating all aspect again for each TestViewModel seems also not be correct.

If you only want to suppress the "OperationCanceledException" then try using an extension method.
Also you can just suppress the OperationCanceledException on the method call.
Consider this code which should be more clearer and explicit:

class Program
    {
        static async Task Main(string[] args)
        {
            var model = new TestViewModel();

            Func<Task> func = () => model.Run();
            await func.SuppressCancel();
        }
    }

    public class TestViewModel
    {
        private CancellationTokenSource _cts;
        
        public async Task Run()
        {
            _cts = new CancellationTokenSource(3000); // cancel after some time for testing purpose

            // simulate long tasks
            for (var i = 0; i < 1000; i++)
                await LongTask(i, _cts.Token); // task won't run in parallel. If this was the intention create all tasks and call Task.WaitAll()
        }
        
        public async Task LongTask(int i, CancellationToken cancellationToken)
        {
            Trace.WriteLine(i + " - work part 1");
            await Task.Delay(500);
            cancellationToken.ThrowIfCancellationRequested();

            Trace.WriteLine(i + " - work part 2");
            await Task.Delay(500);
            cancellationToken.ThrowIfCancellationRequested();

            Trace.WriteLine(i + " - work part 3");
            await Task.Delay(500);
            cancellationToken.ThrowIfCancellationRequested();
        }

        public void Stop()
        {
            Trace.WriteLine("Task will be cancel");
            _cts.Cancel();
            _cts = null;
        }
    }

    public static class TaskExtensions
    {
        public static async Task SuppressCancel(this Func<Task> func)
        {
            try
            {
                var task = func();
                await task;
            }
            catch (OperationCanceledException)
            {
                // suppress cancel
            }
        }
    }

@shikihane
Copy link
Author

Thank you for your wise advice,I will consider your advice very well, That is really universal practice currently. I think it is necessary for me to explain my trouble. I am develop about an automation system that consist of a lot of motion and can stop,play,pause,skip so on.

Therefore, there is a lot of IO behavior and long time calculation in the system. Unfortunately most of the code was inherit form long ago and most do not support TAP pattern.

If I pass a CancellationToken from the top level it means I must to rewrite a lot of code and CancellationToken will diffuse in the whole.That was a frustrating job.

So I m looking for a clean and easier way to add feature .

I want stick to principle about Separation of concerns, but there is easier cause chaos for TAP pattern. Maybe I understood it wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants