2

Работаю с потоками первый раз. Подскажите как решить проблему. Мне нужно в первом потоке сгенерировать строку и записать ее в list, а в другом принять эту строку, отсортировать и записать в другой list. Проблема в том, что программа работает не всегда корректно, то createList сработает два раза, а потом только sortList, то еще как-то. Как можно это решить?

#include<iostream> 
#include<cstdlib> 
#include<vector> 
#include<mutex> 
#include<thread> 
#include<algorithm>
#include<string>
#include<list>
using namespace std;
const int elementsCount = 10;
void PrintList(list<string> list_)
{
    cout << "LIST" << endl;
    for (list<string>::iterator it = list_.begin(); it != list_.end(); it++)
    {
        cout << *it << ' ';
    }
    cout << endl;
}
void createList(list<string> &l, mutex& m_arr, mutex& m_out) {
    srand(time(NULL));
    int count;
    string str;
    char str_char[1];
    for (int i = 0; i < elementsCount; i++) {
        m_arr.lock();
        count = rand() % 8 + 1;
        for (size_t i = 0; i < count; i++)
        {
            str_char[0] = char(rand() % 26 + 0x61);
            str += string(str_char, sizeof(str_char));
        }
        l.push_back(str);
        str.clear();
        m_arr.unlock();
        m_out.lock();
        cout << "1 ------ Create " << l.back() << "\n";
        m_out.unlock();
    }
}
void sortList(list<string> &l, list<string> &l2, mutex& m_arr, mutex& m_out) {
    int i = 0,j=0;
    string str;
    while (i < elementsCount) {
        m_arr.lock();
        if (l.size() > j) {
            str = l.back();
            sort(str.begin(), str.end());
            l2.push_back(str);
            j++;
            m_out.lock();
            cout << " Sort " << l2.back() << "\n";
            m_out.unlock();
            i++;
        }
        m_arr.unlock();
    }
}
int main() {
    mutex m_arr, m_out;
    list<string> l1, l2;
    thread push_thread(createList, ref(l1), ref(m_arr), ref(m_out));
    thread pop_thread(sortList, ref(l1), ref(l2), ref(m_arr), ref(m_out));
    if (push_thread.joinable()) push_thread.join();
    if (pop_thread.joinable()) pop_thread.join();
    PrintList(l1);
    PrintList(l2);
    system("pause");
    return 0;
}

2 ответа 2

0

Так, ну у вас тут ситуация гонки. Вы когда проверяете thread.joinable, может сложиться так, что тред еще не закончен, следовательно, join вы не сделаете - а значит, вы просто не дождетесь того момента, когда тред отработает - и выдадите пустой список.

Решение - уберите проверку на joinable, она лишняя.

Ну и далее по коду, есть очень много проблемных мест, вроде вот этого

const int elementsCount = 10

- глобальная переменная - уже сама по себе, признак плохого дизайна и предвестник проблем в будущем, так здесь она у вас еще и не того типа - как количество элементов может быть отрицательным? Используйте size_t. Да, замечание кажется мелким, но если потом вы сотворите такое в большом, переносимом между архитектурами cpu проекте, вам будет больно.

str += string(str_char, sizeof(str_char));

- str_char можно использовать в виде просто char, не перегружая программу лишним массивом и не заставляя string каждый раз копировать из него

rand() % 26 

- так ограничивать диапазон RAND неправильно, вы на выходе получите вместо почти-равномерного распределения ерунду.

list<string>::iterator 

- раз уж используете c++11, пользуйтесь ranged for или auto

Вместо явных lock и unlock на мьютексе следует использовать lock_guard

3
  • Проверка на joinable там действительно не нужна, но никакой гонки это не вызывает. Это просто абсолютно бесполезная проверка.
    – ixSci
    22 дек 2018 в 10:06
  • @ixSci - тогда почему программа автора не работает?
    – gbg
    22 дек 2018 в 10:39
  • Там каша, которую мне пока что лень разгребать :) Но ошибка точно не в этом.
    – ixSci
    22 дек 2018 в 11:23
0

Ваш код ужасен. Несколько моментов:

  • Вы захватывает мьютексы на слишком длинные периоды, в которых нет смысла. Мьютекс нужно захватывать ровно на то время, которое он нужен.
  • У Вас идёт перекрытие имён (i), а также сами имена ужасы.
  • Проверка на joinable() избыточна и совершенно не нужна.
  • Зачем использовать list? Его интерфейс для этой задачи ужасен.

Теперь по логике кода: у Вас один поток заполняет массив, а второй читает последний элемент массива. Разве проблема не очевидна? Что будет, если первый поток добавит 10 элементов, перед тем, как второй будет иметь шанс отработать? Правильно, второй поток обработает 10 одинаковых элементов.

Такие вещи делаются через условные переменные (condition variables), а не так как делаете Вы. Но Ваш код тоже можно сделать рабочим, для этого достаточно заменить str = l.back();, на вот такой код:

auto itemIter = l.cbegin();
advance(itemIter, j);
str = *itemIter;

Который выглядел бы вот так, если бы использовался vector:

str = l[j];

Если ничего не упустил, то одного этого изменения должно быть достаточно, чтобы всё работало правильно, но качество кода всё равно остаётся вопросом.

Ваш ответ

Нажимая на кнопку «Отправить ответ», вы соглашаетесь с нашими пользовательским соглашением, политикой конфиденциальности и политикой о куки

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.