Problems using multithreading on C#

Question

I want to implement a program that shows some chars moving randomly on console, each of them with different speed.

I created a recursive method that moves one letter randomly on console. When I want to move two letters I use two threads calling the same method.

The program works perfectly the first minutes, but after some time the letters began to appeare everywhere on console!

I'm really sure that my recursive method is okay (I even try to create another method, this time just using a while(i<100000) instead of recursion. But the same error appeared). Could someone help me with this?

Thank you very much.

EDIT: Sorry, here's a sample-code (don't consider what happen if the letters take the same position). The letters move on a 'stadium' they move between 20 - 51 in the x axis and 5 - 26 in the y axis.

public void WriteAt(string s, int x, int y)
    {
        try
        {
            Console.SetCursorPosition(x, y);
            Console.Write(s);
        }
        catch (ArgumentOutOfRangeException e)
        {
            Console.Clear();
            Console.WriteLine(e.Message);
        }

    }

    public void impresion()
    {
        int x = random.Next(20, 51);
        int y = random.Next(5, 26);
        WriteAt("A", x, y);
        imprimir("A", x, y, 80);
    }

    public void impresion2()
    {
        int x = random.Next(20, 51);
        int y = random.Next(5, 26);
        WriteAt("E", x, y);
        imprimir2("E", x, y, 20);
    }

    public void go()
    {
        Thread th1 = new Thread(impresion);
        Thread th2 = new Thread(impresion2);
        th1.Start(); //creates an 'A' that will move randomly on console
        th2.Start(); //creates an 'E' that will move randomly on console
    }

    public void imprimir(string s, int x, int y, int sleep)
    {
        Thread.Sleep(sleep);
        WriteAt(" ", x, y);
        int n = random.Next(1, 5);

        if (n == 1)
        {
            if ((x + 1) > 50)
            {
                WriteAt(s, x, y);
                imprimir(s, x, y, sleep);
            }
            else
            {
                WriteAt(s, x + 1, y);
                imprimir(s, x + 1, y, sleep);
            }
        }

        else if (n == 2)
        {
            if ((y - 1) < 5)
            {
                WriteAt(s, x, y);
                imprimir(s, x, y, sleep);
            }
            else
            {
                WriteAt(s, x, y - 1);
                imprimir(s, x, y - 1, sleep);
            }
        }

        else if (n == 3)
        {
            if ((x - 1) < 20)
            {
                WriteAt(s, x, y);
                imprimir(s, x, y, sleep);
            }
            else
            {
                WriteAt(s, x - 1, y);
                imprimir(s, x - 1, y, sleep);
            }
        }

        else
        {
            if ((y + 1) > 25)
            {
                WriteAt(s, x, y);
                imprimir(s, x, y, sleep);
            }
            else
            {
                WriteAt(s, x, y + 1);
                imprimir(s, x, y + 1, sleep);
            }
        }
    }

Answer 1

There can be a million subtle issues with threading -- anything that access a shared resource must be considered suspect.

Consider that a move-position-followed-by-a-put-character is not atomic and one thread could interrupt another causing a move-move-put-put scenario. In reality the situation is actually worse than this as the control sequences themselves are compromised of multiple bytes sent to the terminal: thus the control sequences themselves may be becoming corrupt!

Use a critical region guard (lock) around the access to the terminal. The lock should encompass all the operations which must be atomic (not interrupted) in relation to eachother:

lock (foo) {
   move(...)
   draw(...)
}

Adapt for the WriteAt function as appropriate.

However, keep in mind that even with this change there is still a subtle race condition, consider:

1. A is cleared.
2. A is drawn (to where E is).
3. E is cleared (was where A was just drawn).
4. E is drawn.

With the above it is possible that (at a particular time) E will appear on the screen while A will not appear. That is, the lock itself, while guarding access to the console, is unable to adequately guard the interactions between the threads and console.

Happy coding.

---

See also What are common concurrency pitfalls? for some general hints and links.

Answer 2

The previous answer regarding locking access to the console will solve your immediate problem.

You really don't need explicit threading for this. You can do it with a couple of timers and some state information. For example:

class CharState
{
    private static Random rnd = new Random();
    private object RandomLock = new object();
    public int x { get; private set; }
    public int y { get; private set; }
    public readonly char ch;
    public CharState(char c)
    {
       ch = c;
       SetRandomPos();
    }

    public void SetRandomPos()
    {
        lock (RandomLock)
        {
            // set x and y
        }
    }
}

The random number generator is shared among all of the CharState object instances. It's protected with a lock in SetRandomPos because Random.Next will fail if called by multiple threads concurrently. Don't worry about the "efficiency" of the lock. It's going to cost you maybe 100 nanoseconds.

Now, create two CharState instances and timers to control them:

CharState char1 = new CharState('A');
CharState char2 = new CharState('X');

System.Threading.Timer timer1 = new System.Threading.Timer(
    MoveChar, char1, 1000, 1000);

System.Threading.Timer timer2 = new System.Threading.Timer(
    MoveChar, char2, 1200, 1200);

Here, the "A" will move once every second, and the "X" will move every 1.2 seconds.

And your MoveChar function becomes:

void MoveChar(object state)
{
    CharState ch = (CharState)state;

    // erase the previous position
    WriteAt(" ", ch.x, ch.y);

    ch.SetRandomPos();
    WriteAt(ch.ch, ch.x, ch.y);
}

There are many benefits to this approach. You don't need separate methods for each character you want to move around, and you can move each character at a different rate. If you want, you can extend the CharState class to give each character a particular area in which to move.

You could do the same kind of thing with explicit threading, but the timer is easier to use, and will consume fewer resources on your system. If you wanted to move 10 different characters around, you would need 10 separate threads, each consuming a large number of resources on your system. That's not good, especially since the threads spend most of their time sleeping--doing nothing.

With timers, on the other hand, the system takes care of spinning up only as many threads are needed to handle concurrent requests. Using timers, you could have 100 different characters moving around and the system would be using only a handful of threads.