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

Рейтинг: 0Ответов: 2Опубликовано: 28.02.2025

Дана следующая задача: С клавиатуры вводится число роботов, собирающих игрушки, и число наборов для сборки. Число наборов для сборки игрушек должно превышать число роботов не менее чем в 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 был, и одновременно с этим они работали параллельно?

c# winforms
Источник: Stack Overflow на русском

Ответы

▲ 0Принят

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

  • здесь не нужен массив 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} закончил работу.");
}

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

▲ 0

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

  • Массив 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 кучу кода, который может прекрасно работать и без него.