Thread sanitizer выдал предупреждение на код, который я много лет считал корректным.
Для нетерпеливых вот самодостаточный пример на godbolt: https://godbolt.org/z/3xPadcnva.
Для всех остальных пояснение:
- на главной нити создается объект actual_repo. В этом объекте живут и std::mutex, и condition_variable (на котором будет осуществляться ожидание);
- ссылка на actual_repo передается в дочернюю нить. Через какое-то время дочерняя нить вызывает для actual_repo метод stop;
- главная же нить засыпает на вызове wait_for_stop у объекта actual_repo. Этот метод вернет управление только после того, как дочерняя нить вызовет stop;
- когда дочерняя нить вызывает stop, то главная нить просыпается, выходит из wait_for_stop, после чего разрушается объект actual_repo;
- после чего дожидаемся завершения дочерней нити и прекращаем работу.
Фокус здесь в том, что внутри stop условная переменная взводится (вызов notify_one()) без захвата мутекса.
А это ведет к тому, что главная нить может проснуться и уничтожить объект actual_repo еще до того, как дочерняя нить завершит вызов stop.
Т.е. деструктор для repo_basic::m_stop_initiated_cv может отработать еще до того, как на дочерней нити завершится вызов m_stop_initiated_cv.notify_one().
И как раз thread sanitizer и ругается на то, что в главной нити происходит модификация содержимого repo_basic::m_stop_initiated_cv тогда как на дочерней нити мы это содержимое только только прочитали.
Полный код примера под катом. Если раскомментировать строчку в repo_basic::try_initiate_stop, то thread sanitizer будет доволен и ничего не диагностирует.
Собственно, у меня два (возможно, риторических) вопроса к аудитории:
- это ложное срабатывание thread sanitizer-а или же в таком коде реально есть проблема гонок? Я лично всегда думал, что здесь гонок нет. Но есть вероятность, что ошибаюсь. А так как ошибаюсь часто, то уже сомневаюсь даже в том, в чем раньше был уверен;
- если это таки ложное срабатывание, то как тогда быть с кодом, чтобы его можно было проверять thread sanitizer-ом, но не натыкаться на это ложное срабатывание снова и снова?
В качестве дополнения: в примере максимально выхолощенная версия, выделенная из реального кода. Поэтому там есть и repo_basic, и другие усложнения. Полагаю, что для демонстрации данного случая можно было и более простой код написать. Однако, оригинальный код специально написан с учетом того, что после метода stop к actual_repo обращаться нельзя (т.е. уже учтено, что actual_repo может быть разрушен параллельно работающей нитью). Да и сам метод stop в actual_repo специально написан так, что последнее, что он делает -- это обращается к m_stop_initiated_cv.notify_one().
|
#include <thread> #include <mutex> #include <condition_variable> #include <chrono> #include <iostream> class repo_basic { protected: std::mutex m_lock; std::condition_variable m_stop_initiated_cv; int m_status{ 0 }; int try_initiate_stop() { std::lock_guard< std::mutex > lock{ m_lock }; if( !m_status ) { m_status = 1; return m_status; } return -1; } public: void wait_for_stop() { std::unique_lock< std::mutex > lock{ m_lock }; m_stop_initiated_cv.wait( lock, [this]{ return 0 != m_status; } ); } }; class actual_repo : protected repo_basic { public: using repo_basic::wait_for_stop; void stop() { const auto r = this->try_initiate_stop(); if( 1 == r ) { // std::lock_guard< std::mutex > lock{ this->m_lock }; this->m_stop_initiated_cv.notify_one(); } } }; int main() { std::thread child_task; { actual_repo repo; child_task = std::thread{ [&repo]() { // Take some time to the main thread to call wait_for_stop. std::this_thread::sleep_for( std::chrono::milliseconds{ 250 } ); repo.stop(); } }; repo.wait_for_stop(); } child_task.join(); std::cout << "successful completion" << std::endl; } |
Комментариев нет:
Отправить комментарий