суббота, 18 мая 2019 г.

[prog.c++] Еще один любопытный баг на стыке многопоточности и ООП

Появился повод рассказать об еще одной гримасе многопоточности: на днях был найден и исправлен любопытный баг, в котором вроде бы корректная работа с многопоточностью наложилась на особенности реализации ООП.

Итак, обнаружилось, что один из тестов время от времени падает с диагностикой "pure virtual method called". Разбирательство показало, что проблема проявляется в коде, который похож вот на этот (лишние детали убраны, дабы не можно было рассказывать только о сути проблемы):

class data_owner_t {
public:
   virtual void update() = 0;
   ...
};

class data_repository_t {
   std::mutex lock_;
   some_container_t<data_owner_t *> owners_;
   ...
public:
   void add(data_owner_t & owner) {
      std::lock_guard lock{lock_};
      owners_.insert(&owner);
   }

   void remove(data_owner_t & owner) {
      std::lock_guard lock{lock_};
      owners_.erase(&owner);
   }

   void update_all() {
      std::lock_guard lock{lock_};
      for(auto * p : owners_)
         p->update();
   }
   ...
};

Виртуальный метод здесь всего один -- это data_owner_t::update. Вызывается он только внутри data_repository_t::update_all, в цикле, перебирающем всех зарегистрированных owner-ов. Значит в какой-то момент времени внутри data_repository_t оказывается невалидный указатель на owner-а. Но как и почему?

Практически сразу же стало понятно, что проблема возникает если пересекается изъятие owner-а и итерация по списку owner-ов. Ощущение было такое, что метод data_repository_t::remove успешно отрабатывает в тоже самое время, пока работает data_repository_t::update_all. Поэтому в цикле внутри update_all мы время от времени натыкаемся на уже переставший быть актуальным указатель.

Это была бы хорошая версия, но она вела в настоящий тупик: было совершенно непонятно, как работа remove и update_all могла пересечься, если они обе защищены одним и тем же мутексом. Тем не менее, было очевидно, что в списке owner-ов оказывается невалидный указатель. Т.е. происходило то, что вроде как не должно было происходить. Но происходило.

А дело оказалось вот в чем.

Часть наследников data_owner_t выполняли добавление самих себя в репозиторий в конструкторе, а изъятие в деструкторе. Чтобы не повторять одно и тоже снова и снова, был сделан вспомогательный класс по типу вот такого:

class autostarted_data_owner_t : public data_owner_t {
   data_repository_t & repo_;
public:
   autostarted_data_owner_t(data_repository_t & repo) : repo_{repo} {
      repo_.add(*this);
   }
   ~autostarted_data_owner_t() override {
      repo_.remove(*this);
   }
};

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

Далее реальные классы owner-ов наследовались от этого autostared_data_owner_t. Поэтому экземпляры реальных owner-ов автоматически добавлялись в репозиторий при создании и изымались из репозитория при удалении. И проблема была как раз в автоматическом изъятии при удалении.

Когда реальный owner удаляется, то сперва вызывается его собственный деструктор. Потом начинают вызваться деструкторы базовых классов. И в какой-то момент времени мы оказывается в деструкторе autostarted_data_owner_t. Здесь мы делаем вызов data_repository_t::remove. И, если в данный момент работает data_repository_t::update_all, то мы ждем завершения update_all. Тут все правильно, указатель на еще существующий объект все еще лежит внутри data_repository.

Но фокус в том, на что именно он указывает? А указывает он не на полноценный объект реального owner-а, а всего лишь на тот "огрызок", который соответствует autostarted_data_owner_t. И у этого огрызка нет реализации метода update. Для него update является чистым виртуальным методом.

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

Так вот, при определенном стечении обстоятельств складывается ситуация, когда хранящийся в data_repository указатель, который раньше указывал на полноценного owner-а, вдруг начинает указывать на огрызок owner-а, соответствующий типу autostarted_data_owner_t. Соответственно, если вызвать метод update в данный момент, то как раз и произойдет вызов чистого виртуального метода. На что, собственно, и довелось наступить :)

Тоже самое, но в обратном порядке, происходит и при конструировании объекта: когда работает конструктор базового класса, то VMT содержит указатели на реализации виртуальных методов из базового класса, при переходе к конструктору производного класса VMT обновляется и начинает указывать уже на реализации виртуальных методов из производного класса). Поэтому такой же баг мог возникнуть не только при пересечении remove и update_all, но и при пересечении add и update_all.

Так что еще раз повторю то, что говорил неоднократно: многопоточнось -- это пот, боль и кровь. Фокусы можно ожидать на каждом шагу. Даже там, где, казалось бы, все корректно закрыто mutex-ами. Поэтому если у вас есть возможность не писать многопоточный код, то не пишите его.

6 комментариев:

Pavel комментирует...

Спасибо, интересный баг. А как вы его исправили, просто добавили реализацию `autostarted_data_owner_t::update()` ?

Вот баг найден, причина понятна, план исправления есть. Хорошо бы добавить регрессионный тест для найденного случая. И тут начинаются трудности с гарантированным воспроизведением ошибки. И с уверенностью, что это исправление действительно работает как запланировано.

eao197 комментирует...

Добавить пустой `update()` в `autostarted_data_owner_t` -- это очевидное решение. Но, имхо, довольно стремное. Т.к. в принципе не хорошо, что `update_all()` может вызвать метод `update()` для объекта, находящегося в процессе разрушения.

Поэтому более надежный способ -- это сделать так, чтобы при вызове `remove()` процесс разрушения owner-а еще даже не запускался бы. Достичь этого можно двумя способами:

1. Вообще отказаться от `autostared_data_owner_t`, как и от практики вызывать `add()` и `remove()` в конструкторах-деструкторах owner-а. Пусть у каждого owner-а будет `start` и `stop`, которые будут вызываться в нужных местах вручную.

2. Сделать обертку вида:

template
class autostarted_holder_t {
data_repository_t & repo_;
Owner data_;
public:
autostarted_holder_t(data_repository_t & repo, ...) : repo_{repo}, ... {
repo_.add(data_);
}
~autostarted_holder_t() {
repo_.remove(data_);
}
...
}

В этом случае при разрушении `autostarted_holder_t` объект Owner еще вполне себе жив и здоров. Уничтожится он лишь после того, как будет изъят из repo_.

eao197 комментирует...

Да, уж. Код в комментариях в blogger-е не напишешь. Суть в том, что autostarted_holder_t -- это шаблон. В качестве параметра -- тип Owner.

Unknown комментирует...

Тут есть еще один нюанс - синхронизация памяти на мьютексе. Внутри update_all() берется мьютекс и состояние памяти между потоками синхронизируется (однако, все объекты в репозитории еще целы), далее в другом потоке вызывается деструктор объекта, который обновляет указатель на vtable и после этого деструктор синхронизируется на мьютексе. Но, поток, в котором работает update_all() только после этого доходит до соответствующего объекта и не синхронизируется (повторно) на том же мьютексе (мьютекс берется один раз).

Вопрос: какой указатель vtable должен увидит update_all(). Тот, что был при его захвате мьютекса или который был затем обновлен в другом потоке в деструкторе? Очень сомневаюсь, что указатель на vtable атомарный (тогда на нем будет синхронизация и все хорошо), однако, архитектура amd64 довольно сильно упорядочена, поэтому тут такое явление увидеть, скорее всего, невозможно.

Однако, возможна такая последовательность: указатель на vtable будет прочитан в потоке update_all(), затем в другом потоке отработают все деструкторы вплоть до ~autostarted_data_owner_t() и указатель на vtable будет изменен, а затем в первом потоке будет использоваться указатель на целый объект, хотя он уже частично разрушен. Да и вообще, ничего не мешает разрушать объект в одном потоке, когда в другом вызывается update(). В общем, Женя, у тебя архитектура не подходящая для многопоточки (деструктор с удалением из репозитория должен работать атомарно).

Unknown комментирует...

Вообще, когда занимался многопоточкой, для себя решил, что обычные методы еще можно стараться сделать thread safe, но деструктор должен вызываться только гарантированно из одного потока, когда объектом точно никто не может пользоваться. Т.е. обычные методы с деструктором никак не должны пересекаться (вызываться в разных потоках). Если вызывается деструктор, то объектом никто не пользуется. Иначе мозг сломается учитывать все варианты (к тому же в деструкторе не понятно, в каком состоянии объект, так что деструктор должен быть однопоточным).

ssko комментирует...

Как всегда крутецкий баг, как всегда из-за ма-аленких нюансов языка. Сидишь и думаешь, что же лучше: мощь C++ или упорядоченность Rust... И вспоминаешь, что уже давно пишешь на C... ))))
Но на C сопоставимую функциональность реализовать - пальцы об клавиши до костей сотрешь...)))