47

I am currently in the process of retrofitting our long-running methods to be cancelable. I am planning on using System.Threading.Tasks.CancellationToken to implement that.

Our methods generally perform a few long-running steps (sending commands to and then waiting for hardware mostly), e.g.

void Run()
{
    Step1();
    Step2();    
    Step3();
}

My first (maybe stupid) thought on cancellation would transform this into

bool Run(CancellationToken cancellationToken)
{
    Step1(cancellationToken);

    if (cancellationToken.IsCancellationRequested)
        return false;

    Step2(cancellationToken);

    if (cancellationToken.IsCancellationRequested)
        return false;    

    Step3(cancellationToken);

    if (cancellationToken.IsCancellationRequested)
        return false;

    return true;
}

which frankly looks horrible. This "pattern" would continue inside the single steps, too (and they are necessarily rather longish already). This would make Thread.Abort() look rather sexy, although I know its not recommended.

Is there a cleaner pattern to achieve this that does not hide away the application logic beneath lots of boilerplate code?

Edit

As an example for the nature of the steps, the Run method could read

void Run()
{
    GiantRobotor.MoveToBase();
    Oven.ThrowBaguetteTowardsBase();    
    GiantRobotor.CatchBaguette();
    // ...
}

We are controlling different hardware units that need to be synchronized to work together.

10
  • Is each step completely independent from other steps?
    – Matten
    Nov 5, 2013 at 14:40
  • 4
    Could you refactor the code to have one method Step(int number)? Such that you can loop through from 1 to 3 and check if the cancellation token is requested only once?
    – azz
    Nov 5, 2013 at 14:41
  • 2
    See stackoverflow.com/questions/3632149/… for a comprehensive comparison of various methods to cancel a thread.
    – Markus
    Nov 5, 2013 at 14:48
  • Could you use mutual exclusion between Thread.Abort() call and critical sections? Then, you'd only have to mutex critical code (saving state perhaps), and let Thread.Abort() kill the rest.
    – neeKo
    Nov 5, 2013 at 14:56
  • 2
    This might not help much, but worth mentioning: make a Monad for it. I think you can implement one in C#. Your case is the one usually given as the first example in Monad tutorials (i.e. chaining functions / steps when one can fail and must abort the chain). Look in the direction of Haskell's Maybe Monad. Seems a good conceptual fit.
    – CamilB
    Nov 6, 2013 at 8:34

8 Answers 8

33

If the steps are somehow independend regarding the dataflow within the method, but can't be executed in a parallel matter, the following approach may be better readable:

void Run()
{
    // list of actions, defines the order of execution
    var actions = new List<Action<CancellationToken>>() {
       ct => Step1(ct),
       ct => Step2(ct),
       ct => Step3(ct) 
    };

    // execute actions and check for cancellation token
    foreach(var action in actions)
    {
        action(cancellationToken);

        if (cancellationToken.IsCancellationRequested)
            return false;
    }

    return true;
}

If the steps don't need the cancellation token because you can split them up in tiny units, you can even write a smaller list definition:

var actions = new List<Action>() {
    Step1, Step2, Step3
};
3
  • +1 NOTE: Should be List<Action<CancellationToken>> but you have the right idea. I was thinking something similar to this.
    – gleng
    Nov 5, 2013 at 14:46
  • 1
    Personally, I think this is not really a good solution. The built-in ThrowIfCancellationRequested pattern is far easier to maintain (throughout potentially many nested methods) and is more readable. Not to mention that framework/outside methods that accept a CancellationToken will likely already throw OperationCanceledExceptions meaning you have to wrap your Run method in a try catch block anyway. I added an answer below. Nov 11, 2013 at 22:23
  • @AndrewHanlon The caller could catch the OperationCanceledException as well or you can just wrap the run-part. I see no relation of your comment to my answer. "It depends" on the way of how the methods are implemented.
    – Matten
    Nov 13, 2013 at 7:47
23

what about continuation?

var t = Task.Factory.StartNew(() => Step1(cancellationToken), cancellationToken)
   .ContinueWith(task => Step2(cancellationToken), cancellationToken, TaskContinuationOptions.OnlyOnRanToCompletion, TaskScheduler.Current)
   .ContinueWith(task => Step3(cancellationToken), cancellationToken, TaskContinuationOptions.OnlyOnRanToCompletion, TaskScheduler.Current);
2
  • 2
    Good answer, this is something Tasks excel at. Nov 5, 2013 at 15:46
  • 1
    But this looks super ugly! Very machine friendly, but barely readable.
    – Harry
    Nov 1, 2016 at 19:00
9

I admit it isn't pretty, but the guidance is to either do what you have done:

if (cancellationToken.IsCancellationRequested) { /* Stop */ }

...or the slightly shorter:

cancellationToken.ThrowIfCancellationRequested()

Generally, if you can pass the cancellation token to the individual steps, you can spread the checks out such that they don't saturate the code. You may also choose not to check for cancellation constantly; if the operations you are performing are idempotent and aren't resource intensive, you don't necessarily have to check for cancellation at every stage. The most important time to check is before returning a result.

If you're passing the token to all of your steps, you could do something like this:

public static CancellationToken VerifyNotCancelled(this CancellationToken t) {
    t.ThrowIfCancellationRequested();
    return t;
}

...

Step1(token.VerifyNotCancelled());
Step2(token.VerifyNotCancelled());
Step3(token.VerifyNotCancelled());
2
  • 2
    Additionally - check for cancellation after iterating over significant data items (e.g. each file in an operation that copies files) and only when you can guarantee successful cleanup.
    – Gusdor
    Nov 5, 2013 at 14:47
  • For my case, these operations are resource intensive, but good advice nonetheless. Thanks =)
    – Jens
    Nov 5, 2013 at 15:18
7

When I had to do something like that, I created a delegate to do it:

bool Run(CancellationToken cancellationToken)
{
    var DoIt = new Func<Action<CancellationToken>,bool>((f) =>
    {
        f(cancellationToken);
        return cancellationToken.IsCancellationRequested;
    });

    if (!DoIt(Step1)) return false;
    if (!DoIt(Step2)) return false;
    if (!DoIt(Step3)) return false;

    return true;
}

Or, if there is never any code between the steps, you can write:

return DoIt(Step1) && DoIt(Step2) && DoIt(Step3);
4

Short version:

Use lock() to synchronize Thread.Abort() call with critical sections.


Let me explain version:

Generally, when aborting thread's you'd have to consider two types of code:

  • Long running code you don't really care if it finishes
  • Code that absolutely has to run it's course

First type is the type we don't care about if the user requested abort. Maybe we were counting to a hundred billion and he doesn't care anymore?

If we'd use sentinels such as the CancellationToken, we would hardly test them in each iteration of the unimportant code would we?

for(long i = 0; i < bajillion; i++){
    if(cancellationToken.IsCancellationRequested)
        return false;
    counter++;
}

So ugly. So for these cases, Thread.Abort() is a godsend.

Unfortunately, as some say, you can't use Thread.Abort() because of the atomic code that absolutely has to run! The code has already subtracted money form your account, now it has to complete the transaction and transfer the money to target account. Nobody likes money disappearing.

Luckily, we have mutual exclusion to help us with this sort of thing. And C# makes it pretty:

//unimportant long task code
lock(_lock)
{
    //atomic task code
}

And elsewhere

lock(_lock) //same lock
{
    _thatThread.Abort();
}

The number of locks will always be <= number of sentinels, because you'd be wanting sentinels on the unimportant code too (to make it abort faster). This makes the code slightly prettier than the sentinel version, but it also makes aborting better, because it doesn't have to wait for unimportant things.


Also to note is that ThreadAbortException could be raised anywhere, even in finally blocks. This unpredictability makes Thread.Abort() so controversial.

With locks you'd avoid this by just locking the entire try-catch-finally block. It the cleanup in finally is essential, then the entire block can be locked. try blocks are generally made to be as short as possible, one line preferably, so we aren't locking any unnecessary code.

This makes Thread.Abort(), as you say, a bit less evil. You might not want to call it on the UI thread, though, as you're now locking it.

2

I am kind of amazed that no one has proposed the standard, built-in, way of handling this:

bool Run(CancellationToken cancellationToken)
{        
    //cancellationToke.ThrowIfCancellationRequested();

    try
    {
        Step1(cancellationToken);
        Step2(cancellationToken);
        Step3(cancellationToken);
    }
    catch(OperationCanceledException ex)
    {
        return false;
    }

    return true;
}

void Step1(CancellationToken cancellationToken)
{
    cancellationToken.ThrowIfCancellationRequested();
    ...
}

While normally you don't want to rely on pushing checks to deeper levels, in this case the steps already accept a CancellationToken and should have the check anyway (as should any non-trivial method accepting a CancellationToken).

This also allows you to be as granular or non-granular with the checks as needed, i.e. before long running / intensive operations.

1

If a refactor is permissible, you could refactor the Step methods such that there is one method Step(int number).

You can then loop through from 1 to N and check if the cancellation token is requested only once.

bool Run(CancellationToken cancellationToken) {
    for (int i = 1; i < 3 && !cancellationToken.IsCancellationRequested; i++) 
        Step(i, cancellationToken);

    return !cancellationToken.IsCancellationRequested;
}

Or, equivalently: (Whichever you prefer)

bool Run(CancellationToken cancellationToken) {
    for (int i = 1; i < 3; i++) {
        Step(i, cancellationToken);
        if (cancellationToken.IsCancellationRequested)
            return false;
    }
    return true;
}
1
  • I believe the return value should also be inverted. Nov 5, 2013 at 14:47
1

You might want to take a look at Apple's NSOperation pattern as an example. It's more complicated than simply making a single method cancelable, but it's pretty robust.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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