Is this code to write to the console forever (and then stop) thread-safe?

Question

Brief

I'm sure you all hate "is this code thread-safe" questions, but I couldn't find a better way to word it.

I've posted a question on CodeReview regarding specific review points, but I'm posting this question on StackOverflow because I think it's a better fit. If I am wrong, please let me know and I will move it CodeReview.

If you prefer to see the code on GitHub, check here

Question

My question amounts to "what problems will I run into using the Task API as below?"--I've never tried working with it before and believe this approach to be erroneous. However, I cannot find an argument for the existence of an error because of my lack of experience. In other words, is this code thread-safe (or whatever appropriate nomenclature fits best), and if not, how can I improve it to be?

Layout

ConsoleLoadingText: class to encapsulate functionality.

Constants
- Defaults for ProductName, LoadingText, and MillisecondsDelay
- Array of spinner pieces
Constructors
- Chained together constructors allow you to provide some or all of the data, relying on defaults to fill in the gaps. They all delegate to one constructor which does the work.
Methods
- Display returns a Task which, when run, does the display work
- Stop stops the display

Code

ConsoleLoadingText.cs

namespace Knoble.Utils
{
    /// <summary>
    /// A class that represents a possibily infinitely looping load screen.
    /// It displays a product name, loading text, and spinner that spins for a given delay.
    /// </summary>
    public class ConsoleLoadingText
    {
        public const string DefaultProductName = "";
        public const string DefaultLoadingText = "Loading...";
        public const int DefaultMillisecondsDelay = 250;

        static string[] spinner = { "|", "/", "-", "\\" };

        readonly string productName, loadingText;
        readonly int millisecondsDelay;

        int i;
        bool @continue = true;

        /// <summary>
        /// Initializes a new instance of the <see cref="T:Knoble.Utils.Loading"/> class.
        /// Defaults to displaying "Loading... x" where the spinner (x) spins every quarter second.
        /// </summary>
        public ConsoleLoadingText () : this (DefaultProductName)
        {
        }

        /// <summary>
        /// Initializes a new instance of the <see cref="T:Knoble.Utils.Loading"/> class.
        /// Defaults to displaying "{productName} Loading... x" where the spinner (x) spins every quarter second.
        /// </summary>
        /// <param name="productName">Product name.</param>
        public ConsoleLoadingText (string productName) : this (productName, DefaultLoadingText)
        {
        }

        /// <summary>
        /// Initializes a new instance of the <see cref="T:Knoble.Utils.Loading"/> class.
        /// Defaults to displaying "{productName} {loadingText} x" where the spinner (x) spins every quarter second.
        /// </summary>
        /// <param name="productName">Product name.</param>
        /// <param name="loadingText">Loading text.</param>
        public ConsoleLoadingText (string productName, string loadingText) : this (productName, loadingText, DefaultMillisecondsDelay)
        {
        }

        /// <summary>
        /// Initializes a new instance of the <see cref="T:Knoble.Utils.Loading"/> class.
        /// Displays "{productName} {loadingText} x" where the spinner (x) spins every {millisecondsDelay} milliseconds.
        /// </summary>
        /// <param name="productName">Product name.</param>
        /// <param name="loadingText">Loading text.</param>
        /// <param name="millisecondsDelay">Milliseconds delay.</param>
        public ConsoleLoadingText (string productName, string loadingText, int millisecondsDelay)
        {
            if (productName == null)
                throw new ArgumentException (nameof (productName));
            if (loadingText == null)
                throw new ArgumentException (nameof (loadingText));
            if (millisecondsDelay < 0)
                throw new ArgumentException (nameof (millisecondsDelay));
            this.productName = productName;
            this.loadingText = loadingText;
            this.millisecondsDelay = millisecondsDelay;
        }

        /// <summary>
        /// Returns a task that, when running, continously prints the loading text.
        /// </summary>
        public Task Display ()
        {
            return Task.Run (() =>
            {
                @continue = true;
                while (@continue)
                {
                    Console.Write ($"\r{productName} {loadingText} {spinner[i]}");
                    i = (i + 1) % spinner.Length;
                    Thread.Sleep (millisecondsDelay);
                }
            });
        }

        /// <summary>
        /// Stop this instance from displaying.
        /// </summary>
        public void Stop ()
        {
            @continue = false;
        }
    }
}

If anyone knows more relevant tags, please suggest or add them — D. Ben Knoble, Jan 09 '17 at 17:49
At the very least if `@continue` is going to be accessed by multiple threads like this, then should be marked volatile. If you are not familiar with volatile however, better advice is to just use a lock around any variables accessed on multiple threads. I believe Sleep creates a full memory fence, so the effects of the lack of thread-safety here might not be apparent in this example. — Mike Zboray, Jan 09 '17 at 18:11

Dave Smash · Answer 1 · 2017-01-09T18:04:59.373

2

Although I haven't tested your code, I suspect that the Thread.Sleep(millisecondsDelay) will block the main thread while it is sleeping, so you may find that your background tasks never seem to do anything. If you want it to run asynchronously so that things stay responsive, you need to use the async keyword and then await Task.Delay(); A quick tutorial on async / await should point you in the right direction.

edited Jan 09 '17 at 18:04

answered Jan 09 '17 at 17:54

Dave Smash

2,941
1
18
38

You cannot `await Thread.Sleep` instead you would do `await Task.Delay` – juharr Jan 09 '17 at 18:02

score 2 · Accepted Answer · edited May 23 '17 at 10:30

It has been already mentioned that changes to @continue may not be seen by background task because read access may get optimized out. volatile will fix the issue, though it is discouraged to use it in favor of other approaches.

But there is also an i field that is both read and written to. Assuming that there can be only one Display task running there won't be problems, but otherwise you should consider more atomic solution to increment it.

Though multiple Display tasks is probably not what you meant by "thread-safety" of this solution.

There are few good advises on writing thread-safe code that we can also consider:

Don't use threading unless necessary - as self-explaining as it can be. Throwing threads at some problem is not always a best decision for a specific problem and it is not easy to do right. Multithreading should only be used when it actually gives some benefits.
But if you have to - don't share mutable memory between threads. If each thread/task gets some data that cannot be mutated in any way and produces independent piece of result, then there is no need to synchronize them.
And even if system requires multithreading and it cannot be expressed as a simple parallel set of independent tasks - don't try to write thread-safe code with simple primitives, rather use standard high level constructs/classes that were already implemented. Thread synchronization is hard - it is very easy to do something wrong.

So:

Not using threading at all - I understand that it is a first dive into TPL, so you have just applied Tasks to some task at hand[no pun intended].

No shared mutable memory - assuming independence of each Display task you can just get rid of that i field. Make it a variable inside method:

var i = 0;
...
    Console.Write ($"\r{productName} {loadingText} {spinner[i]}");
    i = (i + 1) % spinner.Length;
    Thread.Sleep (millisecondsDelay);

Can we do the same with @continue? Kind of, see the next point...

Use standard high level constructs/classes that were already implemented - you are already using Tasks, so why not use everything else designed to work with them. I mean CancellationTokenSource. It is thread-safe and we can both get rid of that @continue and Stop() method.

public Task Display(CancellationToken ct)
{
    return Task.Run (
        () =>
        {
            var i = 0;
            while (!ct.IsCancellationRequested)
            {
                Console.Write ($"\r{productName} {loadingText} {spinner[i]}");
                i = (i + 1) % spinner.Length;
                Thread.Sleep (millisecondsDelay);
            }
        },
        cancellationToken: ct);
}



var tokenSource = new CancellationTokenSource();
var display = consoleLoadingText.Display(tokenSource.Token);

// Emulate some work
Thread.Sleep(10000);

// No need to use Stop method
tokenSource.Cancel();
display.Wait();

...

This is the most comprehensive answer so far. Thank you. – D. Ben Knoble Jan 09 '17 at 19:48 — D. Ben Knoble, Jan 09 '17 at 19:48

score 1 · Answer 3 · answered Jan 09 '17 at 18:13

No, this is not thread safe.

Modern CPUs use a lot of cache and pipeline optimizations. The variable @continue could get end up in a different caches for different threads, meaning your loop would continue forever. To get it into a common cache, use the Volatile keyword.

In addition there is also a chance that two threads will attempt to read and write @continue at the same time. Fortunately it is a Boolean (which is atomic according to the c# reference). If it were any other type you'd want to use some sort of locking mechanism. Variables like millisecondsDelay should be locked if there is any chance it could get updated from some other source. Since you have it marked readonly then you're OK for now.

Is this code to write to the console forever (and then stop) thread-safe?

Brief

Question

Layout

Code

3 Answers3