Проблемы с работой многопоточной программы в области синхронизации работы потоков

Question

Дана следующая задача: С клавиатуры вводится число роботов, собирающих игрушки, и число наборов для сборки. Число наборов для сборки игрушек должно превышать число роботов не менее чем в 100 раз. Каждый робот собирает игрушку за некоторое время, попадающее в диапазон, введенный с клавиатуры пользователем(заменён на случайное число). Когда игрушка собрана, робот берет новый набор для сборки игрушки, пока все наборы не будут собраны. Каждый робот работает в отдельном потоке. Работать в WinForms.

Для этого был создан вот этот код:

public bool stop;
List<Thread> threads = new List<Thread>();
int[] robots;
int numberSets = 0;
Random random = new Random();
object locker = new object();

private void button1_Click(object sender, EventArgs e)
{
    if (Convert.ToInt32(numberRobots.Text) * 100 <= Convert.ToInt32(numberSet.Text))
    {
        robots = new int[Convert.ToInt32(numberRobots.Text)];
        for (int i = 0; i < Convert.ToInt32(numberRobots.Text); i++)
        {
            robots[i] = i + 1;
        }
        for (int i = 0; i < Convert.ToInt32(numberRobots.Text); i++)
        {
            threads.Add(new Thread(new ParameterizedThreadStart(CreatSet)));
            threads[i].Name = $"Робот {i + 1}";
            threads[i].Start(Convert.ToInt32(robots[i]));
        }
    }

    else
    {
        Console.WriteLine("Неправильно!");
    }
}

public void CreatSet(object obj)
{

    int numberRobot = (int)obj;

    while (numberSets != Convert.ToInt32(numberSet.Text))
    {
        int tsb = random.Next(50, 100);
        numberSets++;
        lock (locker)
        {
            var message = $"• {Thread.CurrentThread.Name} начал сбор набора №{numberSets}. Время сбора набора: {tsb} секунд;";
            Invoke((MethodInvoker)delegate
            {
               infAssembly.Items.Add(message);

            });
        }
        var message1 = $"• {Thread.CurrentThread.Name} закончил сбор набора №{numberSets}.";
        Thread.Sleep(tsb);
        Invoke((MethodInvoker)delegate { infAssembly.Items.Add(message1); });

        //Thread.Sleep(random.Next(50, 100));
    }

    var message2 = $"• Наборы для сборки закончились.";
    Invoke((MethodInvoker)delegate { infAssembly.Items.Add(message2); });
}

Вроде как он работает, но выдаёт подобную ахинею, что завершается первым поток с не меньшим временем сборки. Лечиться это удалением lock. Но тут же возникает дилемма, что этот lock зачем-то в этой программе нужен по требованию препода, вопреки логики заявления "Роботы должны работать параллельно друг другу". Короче, как мне переписать программу так, что бы и lock был, и одновременно с этим они работали параллельно?

Answer 1

Замечания и предложения:

Массив robots непонятно зачем нужен вообще. Его i-й элемент просто равен i+1, можно это значение нигде не хранить.
После нажатия кнопки её лучше дизейблить, иначе на неё могут нажать ещё раз и у вас вообще всё запутается. А после окончания работы потоков кнопку можно обратно разрешить.
После того, как вы создали и запустили все потоки, окончания их работы нужно дождаться, для этого и создаётся список threads. Нужно точно так же пройтись по этому списку и сделать Join() каждому потоку.
Непонятно, почему для потоков вы используете список, а для robots массив. При том, что используются они у вас практически однотипно - для доступа по индексу, при заранее известном числе элементов. Это лишний раз показывает, что вы не знаете, какие коллекции для чего нужны и используете их случайным образом.
Под блокировкой нужно делать изменение общих переменных (и коллекций, если таковые есть), которыми совместно пользуются потоки. При этом для использования Invoke (чтобы изменить элементы UI) блокировка не нужна, вы и так с его помощью всегда окажетесь в главном потоке, в котором работает UI.
Блокировку нужно делать на максимально короткое время и на минимально возможный кусок кода. Поэтому никаких "запихнуть в него весь цикл". Размещайте внутри блокировки только тот код, которому реально нужна блокировка. То есть тот код, который работает с общими переменными потоков. Сам цикл не работает с такими переменными, у него своя переменная цикла.
Не нужно на каждой итерации цикла лезть в UI и конвертировать значение Convert.ToInt32(numberSet.Text), лучше сделайте эту конвертацию сразу после нажатия кнопки и положите результат в переменную, которой будете дальше пользоваться в цикле.
Ну и чтобы обойтись без блокировки всего цикла, сделайте другой цикл, такой, чтобы блокировку можно было локализовать в одном месте:

    while (true)
    {
        lock (locker)
        {
            if(numberSets >= здесь_заранее_сконвертированная_переменная)
                 break;
            numberSets++;
        }
        ...

Да, и ещё вам нужно будет запомнить изменённое значение numberSets в локальную переменную, чтобы дальше можно было её вывести в текущем виде, не изменённом другими потоками. Опять же, чтобы не пришлось оборачивать в lock кучу кода, который может прекрасно работать и без него.

Answer 2

Много всего лишнего:

здесь не нужен массив robots - вы его не используете
здесь не нужен список threads - вы его не используете

Вот мой вариант

private readonly Random random = new Random();
private readonly object locker = new object();
private int totalSets = 0;
private int completedSets = 0;

private void button1_Click(object sender, EventArgs e)
{
    int robots = int.Parse(numberRobots.Text);
    totalSets = int.Parse(numberSet.Text);
    completedSets = 0;

    if (robots * 100 >= totalSets) // здесь у вас была ошибка в условии
    {
        for (int i = 0; i < robots; i++)
        {
            Thread robot = new Thread(RunRobot);
            robot.Name = $"Робот {i + 1}";
            robot.Start();
        }
    }
    else
    {
        MessageBox.Show("Слишком мало наборов!");
    }
}

// этот метод нельзя вызывать из UI потока и не стоит вызывать под локом
private void SendMessage(string message)
{
    Invoke((MethodInvoker)delegate
    {
        infAssembly.Items.Add(message);
    });
}

private void RunRobot()
{
    while (true)
    {
        int currentSet; // <-- вот решение вашей проблемы, надо запоминать локально номер набора
        lock (locker)
        {
            if (completedSets >= totalSets)
            {
                break;
            }
            completedSets++;
            currentSet = completedSets;
        }

        int tsb = random.Next(50, 100);
        SendMessage($"• {Thread.CurrentThread.Name} начал сбор набора №{currentSet}.");
        Thread.Sleep(tsb);
        SendMessage($"• {Thread.CurrentThread.Name} закончил сборку набора №{currentSet} за {tsb} миллисекунд.");
    }

    SendMessage($"• {Thread.CurrentThread.Name} закончил работу.");
}

Всегда стремитесь к простоте кода. Если не уверены, что вам нужен какой-то массив - не создавайте его. Используйте отдельные методы для повторяющихся кусков кода. Используйте переменные, чтобы не повторять одинаковые действия.

БЛОГ НА HUSL

Проблемы с работой многопоточной программы в области синхронизации работы потоков

Ответы (2 шт):