1
\$\begingroup\$

Anybody mind providing an opinion on whether or not this is a good or bad example of Task Cancellation and why. I have my own opinion and I've been told that its baseless, just trying to find out who is off base. I don't think its me, but I am okay with it being me. I just need to know its me so I can evolve if required.

class Program
{
    static void Main()
    {
        ImportSource();
        Console.ReadKey();
    }
    public static void ImportSource()
    {
        var cancellationTokenSource = new CancellationTokenSource();
        var token = cancellationTokenSource.Token;
        var task1 = Task.Run(() => Import(token), token);
        Thread.Sleep(500);
        // if we attempt to cancel the token
        CancelToken(cancellationTokenSource);
    }
    public static void CancelToken(CancellationTokenSource cancellationTokenSource)
    {
        Console.WriteLine("Cancellation in process!");
        cancellationTokenSource.Cancel();
    }
    static void Import(CancellationToken token)
    {
        int i = 0;
        do
        {
            Thread.Sleep(100);
            // check if the token is cancelled
            if (token.IsCancellationRequested)
            {
                Console.WriteLine("Token was cancelled");
                break;
            }
            else
            {
                Console.WriteLine("Importing data is in process!");
                Import(token);
                i++;
            }
           
        } while (i < 1);
    }
}

EDIT: Moved my last comment into body since trying not to influence the opinions isn't allowing for any traction. Here is where I am at with this code:

My biggest problem with the above code is that its not a good example of recursive+iterative cancellation. If you entirely delete the do loop, you get identical results and have a decent recursive cancellation example. On the other hand, the loop implementation could change to i<10, the recursive call could just be deleted and then you would have a decent example of iterative cancellation.

So, how could this be modified to be a very clear and meaningful example of both recursive+iteration cancellation, but still keep it as minimal as possible?

\$\endgroup\$
7
  • 1
    \$\begingroup\$ 1) Is this code you've written yourself? 2) If so, could you elaborate on what you think is the problem/ what area you're looking for feedback on - i.e. what does this code even do - what's the context. See codereview.stackexchange.com/help/on-topic \$\endgroup\$
    – Greedo
    May 28, 2021 at 12:26
  • \$\begingroup\$ I want an unfiltered opinion of what you would think if I proposed the above program (which functions) as a "Task Cancellation Example". I have intentionally left out who wrote it and what my opinions are until I get some answers. I will eventually provide that, I just don't want to influence opinions at this point. \$\endgroup\$
    – Bogatitus
    May 28, 2021 at 18:01
  • 1
    \$\begingroup\$ From a demonstration perspective, which is exactly what this code is representing, I didn't even considered the mingling of Thread & Task as a problem. I will investigate the Task.Delay as a more coherent implementation. \$\endgroup\$
    – Bogatitus
    May 31, 2021 at 10:12
  • 1
    \$\begingroup\$ The question still stands. What do you want to demonstrate with/about CancellationToken? The primary use case? The core concept? The chaining ability? The transfer for all the way down? The exception handling? The Register functionality? The CanBeCanceled usage? etc. \$\endgroup\$ May 31, 2021 at 10:24
  • 1
    \$\begingroup\$ Your confusion is understandable and the answer may be that 2 examples are required, but I was wanting to know if its possible to do both in a clear way. IE, threads being as unpredictable as they are, an ideal example would potentially terminate within the loop check or a recursive check, but not exclusively like the current example does. \$\endgroup\$
    – Bogatitus
    Jun 1, 2021 at 9:12

1 Answer 1

0
\$\begingroup\$

Let me share with you my revised version in two parts.

First part

static async Task Main()
{
    Console.WriteLine("Application has been started");

    var cancellationSignal = new CancellationTokenSource();

    StartLongRunningOperationInBackground(cancellationSignal.Token);
    await CancelLongRunningOperationAfter(cancellationSignal);

    Console.WriteLine("Application has been finished");
}

public static void StartLongRunningOperationInBackground(CancellationToken token)
{
    _ = Task.Run(() => LongRunningIterativeOperation(token), token);

    Console.WriteLine("Long running operation has been started");
}

public static async Task CancelLongRunningOperationAfter(CancellationTokenSource cancellationTokenSource, int cancelAfterInMs = 500)
{
    await Task.Delay(cancelAfterInMs);
    cancellationTokenSource.Cancel();
    
    Console.WriteLine("Cancellation has been requested");
}

Changes

  • I've moved the declaration of the CancellationTokenSource to the Main
    • With that the cooperative nature of cancellation tokens are more clear
  • I've renamed the ImportSource to StartLongRunningOperationInBackground
    • I do believe that profound naming can help eligibility and understanding
  • I've moved the waiting before the cancellation request into the CancelLongRunningOperationAfter
    • I think it increases cohesion because cancellation should be done after that period of time
    • So, they belong together
  • I've discarded the result of the Task.Run
    • This is a fairly common way to express fire and forget

Second part

private const int maxSteps = 100;
static void LongRunningIterativeOperation(CancellationToken token)
{
    for (int currentStep = 0; currentStep < maxSteps; currentStep++)
    {
        //Simulate computation intensive code
        Thread.Sleep(100);

        token.ThrowIfCancellationRequested();

        //Simulate progress report
        Console.WriteLine($"Done: {currentStep + 1} / {maxSteps}");
    }
}

static void LongRunningRecursiveOperation(CancellationToken token, int currentStep = 0)
{
    if (currentStep == maxSteps) return;

    token.ThrowIfCancellationRequested();

    //Simulate computation intensive code
    Thread.Sleep(100);

    //Simulate progress report
    Console.WriteLine($"Done: {currentStep + 1} / {maxSteps}");

    LongRunningRecursiveOperation(token, currentStep + 1);
}

Changes

  • I've created two versions of the same method
    • One which demonstrates the iterative approach
    • And another which does the same but in a recursive fashion
  • I've used ThrowIfCancellationRequested instead of checking the IsCancellationRequested
    • If we don't need to do any cleanup then ThrowIfCancellationRequested is more cleaner solution
  • I've used Thread.Sleep here intentionally
    • This is a blocking operation, which can be used to simulate CPU bound operation
    • It is in alignment with Task.Run, which is designed for CPU bound operations
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thank you Peter. FYI, I've been awake for about 22 hours and still have 7 more hours of obligations that have to be done today. I will analyze this as soon as I can, but it certainly looks interesting and I really appreciate the detailed explanations. Based on my quick review, are we are going with the theory that directly mangling the 2 types together with equal cancellation opportunity would generally be a bad use of the TPL? IE, I should pick a lane and parallelize that unit of work. \$\endgroup\$
    – Bogatitus
    Jun 1, 2021 at 14:39
  • \$\begingroup\$ @Bogatitus To be honest with you I don't get your question. Could you please rephrase it? \$\endgroup\$ Jun 2, 2021 at 7:52
  • 1
    \$\begingroup\$ Peter, those were very nice explanations and I learned a number of things from you. I think I'll end my quest to generically put both iterative and recursive cancellation "work" into the same task. I believe it is possible to demo them concurrently, but maybe not without one of them doing some degree of non-theoretical work. \$\endgroup\$
    – Bogatitus
    Jun 2, 2021 at 20:58

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.