Ещё один способ отстрелить себе ногу, используя std::thread

    Стандарт C++11 принёс в язык стандартный механизм поддержки тредов (их часто называют потоками, но это создаёт путаницу с термином streams, так что я буду использовать оригинальный англоязычный термин в русской транскрипции). Однако, как и любой механизм в C++, этот несёт в себе ряд хитростей, тонкостей и совершенно новых способов отстрелить себе ногу. Недавно на Хабре появлялся перевод статьи про 20 таких способов, но список этот исчёрпывающим не является. Я хочу рассказать ещё об одном таком способе, связанном с инициализацией экземпляров std::thread в конструкторах классов.


    Вот простой пример использования std::thread:


    class Usage {
     public:
      Usage() : th_([this](){ run(); }) {}
      void run() {
        // Run in thread
      }
     private:
      std::thread th_;
    };

    В этом простейшем примере код выглядит корректным, но есть одно любопытное НО: в момент вызова конструктора std::thread экземпляр класса Usage ещё не сконструирован полностью. Таким образом, Usage::run() может быть вызван для экземпляра, часть полей которого (объявленных после поля std::thread) ещё не инициализированы, что, в свою очередь, может привести к UB. Это может быть достаточно очевидно на небольшом примере, где код класса умещается в экран, но в реальных проектах этот капкан может быть припрятан за развесистой структурой наследования. Немного усложним пример для демонстрации:


    class Usage {
     public:
      Usage() : th_([this](){ run(); }) {}
      virtual ~Usage() noexcept {}
      virtual void run() {}
     private:
      std::thread th_;
    };
    
    class BadUsage : public Usage {
     public:
      BadUsage() : ptr_(new char[100]) {}
      ~BadUsage() { delete[] ptr_; }
      void run() {
        std::memcpy(ptr_, "Hello");
      }
     private:
      char* ptr_;
    };

    На первый взгляд, код тоже выглядит вполне нормально, более того, он и работать почти всегда будет как ожидается… до тех пор, пока звёзды не сложатся так, что BadUsage::run() вызовется раньше, чем инициализируется ptr_. Чтобы это продемонстрировать, добавим крошечную задержку перед инициализацией:


    class BadUsage : public Usage {
     public:
      BadUsage() : ptr_((std::this_thread::sleep_for(std::chrono::milliseconds(1)), new char[100])) {}
      ~BadUsage() { delete[] ptr_; }
      void run() {
        std::memcpy(ptr_, "Hello", 6);
      }
     private:
      char* ptr_;
    };

    В этом случае вызов BadUsage::run() приводит к Segmentation fault, а valgrind жалуется на обращение к неинициализированной памяти.


    Чтобы избежать таких ситуаций, есть несколько вариантов решения. Самый простой вариант — использовать двухфазную инициализацию:


    class TwoPhaseUsage {
     public:
      TwoPhaseUsage() = default;
      ~TwoPhaseUsage() noexcept {}
      void start() { th_.reset(new std::thread([this](){ run(); })); }
      virtual void run() {}
      void join() {
        if (th_ && th_->joinable()) {
          th_->join();
        }
      }
     private:
      std::unique_ptr<std::thread> th_;
    };
    
    class GoodUsage : public TwoPhaseUsage {
     public:
      GoodUsage() : ptr_((std::this_thread::sleep_for(std::chrono::milliseconds(1)), new char[100])) {}
      ~GoodUsage() noexcept { delete[] ptr_; }
      void run() {
        std::memcpy(ptr_, "Hello", sizeof("Hello"));
      }
     private:
      char* ptr_;
    };
    
    // ...
    GoodUsage gu;
    gu.start();
    std::this_thread::sleep_for(std::chrono::milliseconds(100));
    gu.join();
    // ...
    Поделиться публикацией

    Комментарии 28

      +2
      Чтобы избежать таких ситуаций, есть несколько вариантов решения.


      Эм… и где они? Я как-то подсознательно ожидал листинга способов или разбор плюсов и минусов того или иного способа, но вы их даже не предоставили. Как-то даже немного обидно получилось :(
        0
        Я представил самый простой вариант. Честно говоря, изначально в статье был ещё вариант с запуском треда и ожиданием на condition_variable, но я не увидел у него преимуществ по сравнению с обычной двухфазной инициализацией и удалил.
          0
          Если начать серьезно занудствовать, то мне вариант с двойной инициализацией не нравится хотя бы тем, что нарушает SRP и инкапсуляцию, предоставляю какую-никакую информацию о своей реализации, что не очень архитектурно красиво :)
          Edit: впрочем, местами может быть оправдано, например в случае запуска сервера или еще чего-то, где метод start() будет частью интерфейса.
            0
            Ну, если занудствовать, то треды вообще слабо вписываются в ООП. ))
            0
            Пожалуйста, никогда не надо делать двухфазную инициализацию, хотя бы торчащую наружу пользователю. И дело даже не в SRP всяких, а в том, что для таких классов его сконструированность не означает готовность к использованию, что сильно ломает возможность рассуждать о коде.

            Функцию-фабрику там какую-нибудь лучше сделать или что-то такое.
          +7
          class Usage {
           public:
            Usage() : th_([this](){ run(); }) {}
            virtual ~Usage() noexcept {}
            virtual void run() {}
           private:
            std::thread th_;
          };
          

          Вообще это частный случай более общего антипаттерна с вызовом виртуального метода из конструктора, просто, скажем так, опосредованным образом.
            0
            Честно говоря, не очень понял. В общем случае если вызвать виртуальный метод из конструктора, будет просто применено статическое связывание вместо динамического. В данном же случае вызывается именно run() из производного класса, что и приводит к проблемам.
            Если я что-то не понял в комментарии, поясните пожалуйста.
              +1
              Честно говоря, не очень понял. В общем случае если вызвать виртуальный метод из конструктора, будет просто применено статическое связывание вместо динамического.

              В общем случае да, «ничего такого» не произойдет, но это не то, для чего существуют виртуальные методы. Поэтому по-хорошему вызовы виртуальных методов из конструктора и деструктора — это антипаттерн.
                0
                Кстати, в Java с этим все еще «круче», там же все методы виртуальные, кроме final. Пример:

                public class Base
                {
                  public Base()
                  {
                    System.out.println("Base()");
                    Method();
                  }
                
                  protected void Method()
                  {
                    System.out.println("Method() from Base");
                  }
                }
                
                public class Derived extends Base
                {
                  private int Field = 10;
                  
                  public Derived()
                  {
                    System.out.println("Derived()");
                    System.out.println("Field = " + Field);
                  }
                
                  @Override
                  protected void Method()
                  {
                    System.out.println("Method() from Derived");
                    System.out.println("Field = " + Field);
                  }
                }
                
                [...]
                
                Derived d = new Derived();
                

                выведет:

                Base()
                Method() from Derived
                Field = 0 < — даже инициализатор при объявлении еще не сработал
                Derived()
                Field = 10

                Уот так уот. Нога простреливается на изичах — «ну как же так, я же инициализировал эту переменную даже не в конструкторе, а аж при объявлении, откуда тут NPE???» От верблюда.

                В C#, кстати, было бы примерно то же, но Field в Method() from Derived уже был бы 10. И то радует.
                  0

                  В C# инициализация полей происходит до вызова конструктора.
                  Поэтому отстрелить ногу можно и там.
                  Например, вызвав конструктор объекта раньше статического конструктора.

                    0
                    Каким образом вы собираетесь вызвать конструктор объекта раньше статического конструктора?
                    Static Constructors
                    A static constructor is called automatically to initialize the class before the first instance is created or any static members are referenced.
                      +1
                          public class Program
                          {
                              public static void Main(string[] args)
                              {
                                  //Your code goes here
                                  Console.WriteLine("from main:" + Some.what);
                              }
                      
                              public class Some{
                                  public static readonly Some instance = new Some();
                                  public static readonly string what = "Hello, world!";
                      
                                  static Some(){
                                      Console.WriteLine("From static: " + what);
                                  }
                      
                                  public Some(){
                                      Console.WriteLine("From ctor:" + what);
                                  }
                              }
                          }

                      Онлайн компилятор говорит, что конструктор объекта вызывается раньше:


                      From ctor:
                      From static: Hello, world!
                      from main:Hello, world!
                        0
                        Да, действительно, вы правы. Это частный случай, когда экземпляр класса является статическим полем/свойством этого же класса.
                        Получается, что по приведенной выше ссылке Microsoft себе противоречит, но здесь описывает правильно:
                        Static Classes and Static Class Members
                        Static members are initialized before the static member is accessed for the first time and before the static constructor, if there is one, is called.
              +7

              Двухфазная инициализация в языке с RAII — знатный антипаттерн. (За исключением специальных случаев и весомых причин, само собой.) Означенная проблема с тредом — проблема архитектуры класса. Тред не является частью логики класса и нарушает SRP. Правильнее и чище держать класс отдельно, с интерфейсом пригодным быть использованным в треде. И тестировать проще, чем с тредами возиться в тестах.
              Утрированный пример(откуда следует, что низкоуровневыми примитивами лучше вообще не пользоваться в пользу std::async()):


              struct A{
               void use(){};
              }
              
              ...
              A a;
              std::thread th(&A::use, a); 
              do_other_job();
              th.join();
              • НЛО прилетело и опубликовало эту надпись здесь
                0
                Кстати, похожие проблемы могут появиться в Java — при работе с потоками в конструкторе.
                А именно — если у объекта есть final поля, то гарантируется, что после вызова конструктора их значения будут правильными для всех потоков. Но это неверно, если ссылка на объект была куда-то (например в другой поток) передана внутри конструктора.
                  –2
                  У std::thread есть default constructor.
                  На мой взгляд более простое решение — просто запустить поток в теле конструктора, там уже все members проинициализированы.
                  И не забыть, что деструктор std::thread позовет terminate, если не сделать join к нему (или detach).
                  И unique_ptr тоже не нужен, ибо есть operator=, который сделает move.

                  Примерно так: onlinegdb.com/By5-qJkuN
                  #include <thread>
                  #include <string>
                  #include <iostream>
                  
                  struct test_t
                  {
                      test_t()
                          : str_{"hello world"}
                      {
                          t_ = std::thread([this]{
                              std::cout << "str = " << str_ << std::endl;
                          });
                          t_.detach();
                      }
                      
                  private:
                      std::thread t_;
                      std::string str_;
                  };
                  
                  int main()
                  {
                      test_t t;
                      getchar();
                  
                      return 0;
                  }
                  
                    0
                    Если вы собираетесь внутри лямбды, переданной в std::thread, вызывать виртуальные методы из test_t (как делает автор в оригинальном коде), то это не спасет, вы точно так же отстрелите себе ногу по самые яйца.
                      0
                      Не собираюсь.
                      Комментирую оригинальный посыл статьи, про рождение тредов из initialization list конструктора.

                      Тема вызова виртуальных функций из конструктора — не связана с «как отстрелить себе ногу с std::thread».
                    0
                    Особенно прекрасно будет если из конструктора базового класса вылетит исключение, а потом вызовется run() у уже удаленного ( а точней даже и не созданного ) объекта.
                      0

                      Полагаю, что разделение runnable объектов и самих тредов, частично решит проблему. Т.е. в схеме BaseRunnable::run() -> DervedRunnable::run(), а потом std::thread(&Derived::run, derived) не должно быть проблем.

                        0
                        Насколько я понял, это просто фундаментальные ошибки. Если убрать треды из обоих верхних примеров, то проблема никуда не уйдёт.
                        Вызывать функцию класса из списка инициализации для поля можно только такую, которая использует лишь поля класса, что были уже инициализированы (и стоят выше этого поля).
                        Вызывать виртуальные функции из конструктора и деструктора вообще нельзя (вызов ведёт на ещё не построенный или уже разрушенный «верхний этаж»).
                          0
                          Вызывать виртуальные функции из конструктора и деструктора вообще нельзя (вызов ведёт на ещё не построенный или уже разрушенный «верхний этаж»).

                          Не ведёт, т.к. применяется статическое связывание вместо динамического. Это уже обсуждалось в комментариях выше.
                          Хотя, в целом скорее соглашусь — проблема не завязана только на треды. Я лишь описываю достаточно типичный случай, когда с этой проблемой можно столкнуться, используя треды.
                            +1
                            Посмотрел стандарт — хм, да, действительно, не ведёт. Согласен. Спасибо, буду знать.
                          +1
                          Как по мне все проблемы исчезают, если мы будем использовать функтор вместо класса с run методом.
                          #include <thread>
                          
                          class Runner
                          {
                          public:
                          	void operator()()
                          	{
                          		// Do some staff
                          	}
                          };
                          
                          class ScopedThread
                          {
                          public:
                          	ScopedThread(std::thread&& aThread)
                          		: myThread(std::move(aThread))
                          	{
                          
                          	}
                          
                          	~ScopedThread()
                          	{
                          		if (myThread.joinable())
                          			myThread.join();
                          	}
                          private:
                          	std::thread myThread;
                          };
                          
                          void main()
                          {
                          	Runner()(); // Runs in current thread
                          	ScopedThread(std::thread{ Runner()}); // Runs in separate thread 
                          }
                            0

                            Согласен. Кстати, в Qt так и рекомендуют делать с их потоками. Там чуть другое апи, но суть та же.

                            +3
                            в 2019-м году реализовывать свой thread класс с виртуальным методом run — моветон неслыханный. Вместо наследования от такого AbstractThread и переопределения run можно просто создать std::thread, параметризованный лямбдой.
                              0
                              Так я и не рекламирую такой способ. Конечно, при написании нового кода есть варианты получше. ;)

                            Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                            Самое читаемое