Russian Qt Forum

Добрый день

Вот разбираюсь в обильном чужом коде. Есть система "менеджеров" (читай синглтонов) которые хранят мапы имя->объект одного типа. Пример

C++ (Qt)
// создаем skeleton
Skeleton * sk1  = new Skeleton("Skeleton 1");
 
// регистрируем его в менеджере
scene->getSkeletonManager()->addSkeleton(sk1);
 

Теперь мы можем брать объект по имени и удалять его

C++ (Qt)
Skeleton * sk =  scene->getSkeletonManager()->getSkeleton("Skeleton 1");
scene->getSkeletonManager()->removeSkeleton("Skeleton 1");
 

Подобную организацию я видел не раз, не скажу что это "плохо".

НО - тут же начинаются пляски с шаред пойнтером. Вот меня это как-то смутило. Ведь если "менеджер" берет на себя "ownership" то все удаления должны идти через него. С др стороны шаред грохнет если ноль ссылок.

Как Вы думаете - могут эти 2 подхода "мирно сосуществовать"? Или это несомненно "косяк архитектуры"?

Спасибо

Если деструктор Skeleton корректно сообщает менеджеру о своей "смерти", то все ок.

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

Если возможен многопоточный доступ к менеджеру, то такой указатель в любой момент может стать невалидным (из-за возможности удаления) и это "косяк архитектуры". Если заменить простые указатели на shared_ptr, то это решит проблему с валидностью объекта, но может создать проблемы (не обязательно) с перекрестными ссылками. Поэтому лучше вместо простых указателей использовать weak_ptr, преобразуя их в shared_ptr во время работы с объектом.

В данном случае - если многопоточный доступ отсутствует, то в данных ограничениях решение валидное; если многопоточный доступ присутствует, то имеется "косяк архитектуры".

Тогда это будет косяк имплементации, а не архитектуры.
С "архитектурой" тут проблем нет.

Согласен ;)

"Многопоточность" отсутствует.
Если имеется ввиду "менеджер хранит weak_ptr", то

1) Сейчас менеджер грузит объекты не заботясь о том кто их юзает (может пока и никто). Чем заменить этот ф-ционал если он хранит weak?

2) Что делать с

C++ (Qt)
scene->getSkeletonManager()->removeSkeleton("Skeleton 1");

? Через weak_ptr никого не удалить

В общем так менеджер теряет ownership и становится чем-то типа "наблюдающего" (пусть и полезного)

Житейски разумно, но явно "не айс" (як каже молодь). Чего это "младший" дает указания "менеджеру", вообще чего он знает о существовании менеджера?

И это не решает коллизии "менеджер грохает используемого"

Ни в коем случае). Менеджер хранит shared_ptr, а вот предоставляет weak_ptr. Тогда в месте использования можно усилить weak_ptr до shared_ptr и локально контролировать время жизни объекта. Даже если менеджер удалит объект, грохнется он только после удаления последнего shared_ptr.

Менеджер при создании объекта "говорит" ему, кто хозяин:

Skeleton* Manager::AddSkeleton()
{
  Skeleton* slave = new Skeleton();
  slave->setParent(this);
  return slave;
}

А при смерти slave в этом случае он просто получает информацию о событии:

Skeleton::~Skeleton()
{
  if (m_parent)
    m_parent->slaveKilled(this);
}

Тогда никаких проблем не возникнет.


Тогда менеджер просто получит вызов slaveKilled с поинтером на прибиваемый объект, но он же об этом в курсе :)

да. могут.
shared_ptr позволяет гибкие стратегии владения ресурсами.

не факт.
зависит от бизнес-логики менеджера.

К слову: а по-моему с гибкостью у него проблемы. Напр я рассчитывал на удаление объекта, но по каким-то причинам его нет. Как узнать "кто держит"?

Ну ладно, так что - менеджер должен хранить (вместо обычных указателей как сейчас)

C++ (Qt)
typedef std::map<std::string, shared_ptr<Skeleton> > TMap;

Тогда все что попало в менеджер остается неубиенным. А если менеджер удалит свой шаред из мапы - еще хуже: значит объект уже загружен, а в менеджере его нет - действующий через менеджер вероятно загрузит еще копию. Где же (гибкая) стратегия владения?

Менеджер не в курсе в каком(их) шаред пойнтере хранится этот поинтер
Есть одна система управления/ownership  (менеджер). Вы предлагаете добавить еще другую (parent-child).. Это будет работать только в случае m_parent == manager. Тогда зачем заводить m_parent, slaveKilled и.т.п. если можно действовать через тот же синглтон

C++ (Qt)
Skeleton::~Skeleton( void )
{
 ...
 scene->getSkeletonManager()->takeSkeleton(this->getName());
}
 

shared+weak в манагере.

ничего не понял.
что значит "рассчитывал на удаление объекта" ?

http://www.cyberforum.ru/cpp-beginners/thread1500101.html#post7882976

Это не другая система, а гарантия того, что будет корректно работать "убивание" объектов не через менеджер.
Естественно, m_parent == manager - это основное для этого условие.
m_parent не должен быть указателем на класс менеджера, он может просто указывать на интерфейс типа IManager, у которого есть метод  типа IManager::OnObjectDestroyed(IObject* obj).
Тогда никакой синглтон не нужен. Тем более, в этом случае менеджеров может быть и несколько, не обязательно один :)

Ага, все получается, причем совсем несложно

C++ (Qt)
struct SkeletonManager {
	typedef std::shared_ptr<Skeleton> TShared;
	typedef std::weak_ptr<Skeleton> TWeak;
 
	bool AddSkeleton( const std::string & key, Skeleton * sk )
	{
		// check already in map and alive
		TMap::iterator it = mMap.find(key);
		if (it != mMap.end()) 
			if (!it->second.second.expired()) 
				return false;
 
		// create shared + weak
		TPair & p = mMap[key];
		p.first.reset(sk);
		p.second = TWeak(p.first);
		return true;
	}
 
	bool DeleteSkeleton( const std::string & key )
	{
		TMap::iterator it = mMap.find(key);
		if (it == mMap.end()) return false;
		TPair & p = it->second;
 
		// release shared
		if (p.first.get())      
			p.first.reset();
 
		// get rid pf empty slot
		if (p.second.expired())
			mMap.erase(it);
 
		return true;
	}
 
	TWeak GetSkeleton( const std::string & key )
	{
		TMap::iterator it = mMap.find(key);
		if (it != mMap.end()) {
 
			// return weak if alive
			if (!it->second.second.expired())
				return it->second.second;
 
			// get rid of empty slot
			else
				mMap.erase(it);
		}
		return TWeak();
	}
 
private:
	typedef std::pair<TShared, TWeak> TPair;
	typedef std::map<std::string, TPair> TMap;
	TMap mMap;
};
 

AddSkeleton - клиент отвечает за создание объекта и уникальность ключа, обязан что-то делать если AddSkeleton вернул false (напр попробовать с др ключом/именем)

DeleteSkeleton - обнуляет shared, но используемый объект остается в мапе и доступен через GetSkeleton до тех пор пока все клиенты его не освободят

GetSkeleton - возвращает weak, клиент решает делать ли его shared (захватывать ли ресурс)

Кстати необязательно плодить менеджеров, достаточно одного на все типы, надо в качестве ключа взять пару typeid(T).name() + string. Ну это может и не очень хорошо т.к. напр SkeletonManager имеет ф-ционал специфичный для Skeleton

Нет, не так shared и weak работают - они работают в связке. weak автоматически обнуляется, если ни одного shared больше нет.
Должно быть что-то типа этого (написал прямо здесь, возможны синтаксические ошибки)

C++ (Qt)
struct SkeletonManager {
	typedef std::shared_ptr<Skeleton> TShared;
	typedef std::weak_ptr<Skeleton> TWeak;
 
	bool AddSkeleton( const std::string & key, Skeleton * sk )
	{
		// check already in map and alive
		TMap::iterator it = mMap.find(key);
		if (it != mMap.end()) 
			return false;
 
		// create shared + weak
		mMap[key] = TShared( sk );
		return true;
	}
 
	bool DeleteSkeleton( const std::string & key )
	{
		TMap::iterator it = mMap.find(key);
		if (it == mMap.end())
			return false;
		mMap.erase(it);
 		return true;
	}
 
	TWeak GetSkeleton( const std::string & key )
	{
		TMap::iterator it = mMap.find(key);
		if (it != mMap.end())
			return it.second;
		return TWeak();
	}
 
private:
	typedef std::map<std::string, TSahred> TMap;
	TMap mMap;
};
 
 

А еще лучше, чтобы избежать возможности явной работы с указателем на Skeleton.

C++ (Qt)
	template < _Skeleton >
	bool AddSkeleton( const std::string & key )
	{
		// check already in map and alive
		TMap::iterator it = mMap.find(key);
		if (it != mMap.end()) 
			return false;
 
		// create shared + weak
		mMap[key] = std::make_shared< _Skeleton >();
		//mMap[key] = TShared( new _Skeleton );
		return true;
	}
 

Работа в программе будет выглядеть как

C++ (Qt)
SkeletonManager manager;
...
SkeletonManager::TShared skeleton =  manager.GetSkeleton( key );
if ( skeleton )
{
    ...
}
 
 

Не понял - Вы же сами (совершенно верно) рекомендовали возвращать weak, Я так и сделал - удаление обнуляет shared, но weak может еще жить

Без weak мы ничего не достигаем - мапа не хранит объекты которые могут существовать

Не раз задумывался над подобным. С одной стороны да, хочется обеспечить чтобы все железно шло только через менеджера - иначе всегда остается вероятность ошибки/просмотра. Но на практике это часто неудобно. Тот же Skeleton - конструктор создаст его "пустым", потом его надо грузить (напр из файла), и необязательно он успешно загрузится. Потом апдейтить и.т.п. Тогда чего совать "неконсистентный" объект в глобальную мапу? А в multithreaded (пусть его здесь и нет) это уже прямая ошибка.

Manager хранит shared, но возвращает то weak при вызове GetSkeleton. Сформированный weak и хранимый shared взаимосвязаны (взаимопреобразуемы) - как только shared будет удален (обнулен) все связанные с ним weak автоматически обнулятся. Так как shared и weak взаимопреобразуемы, то нет необходимости в одновременном хранении shared и weak.

В стандарте С++11 и выше можно использовать переменное количество аргументов шаблона для вызова произвольного конструктора Skeleton

Есть. Пример

- клиент получает объект (GetSkeleton) и делает его shared,
- клиент удаляет объект из мапы (DeleteSkeleton), но это не ведет к уничтожению объекта
- клиент опять запрашивает GetSkeleton, в мапе его уже нет (в Вашем сценарии), получив NULL клиент вероятно создает новый объект, загружает его с диска и добавляет в менеджер

Итог: менеджер позволяет одновременное существование одинаковых объектов (или по крайней мере с 1 ключом)

Если клиент удаляет объект из мапы (DeleteSkeleton), то это ведет к моментальному или отложенному уничтожению объекта. Отложенное удаление возникает только в случае, если какой-либо клиент(ы) временно преобразовал weak указатель в shared для возможности использования. Как только все shared удаляются, уничтожается и объект (в традиционном случае). Наличие weak указателей на уничтожение объекта никак не влияет.

Фактически возможно кратковременное существование двух и даже более подобных объектов - это плата за отсутствие синхронизации. Без явной синхронизации достигается так называемая "согласованность в конечном счете". Если требуется строгое наличие только одного экземпляра, то необходимо применять методы явной синхронизации уже с их недостатками (блокировать работу с менеджером до полного удаления скелетона).

В любом случае необходимо защитить мапу от одновременного доступа Mutex или ReadWriteLocker.

Могу отметить, что оба подхода в экстремальной ситуации (перегрузка процессора) ведут себя по разному плохо. Однако согласованность в конечном счете лучше параллелизуется. Но если есть другие ограничения, например по оперативной или графической памяти, то второй вариант может быть и лучше. Здесь все зависит от конкретных условий функционирования ПО.

В данном случае этой платы можно легко избежать. К тому же будет ли это существование кратко (или долго) временным - решает клиент.

Наверное атомарный мутекс здесь бы лучше подошел. Multi-threaded нет и в обозримом будущем не предвидится. Есть десятка 2 "контроллеров", некоторые из них сложные, но многим нужны те же скелетоны и др ресурсы. Использование менеджеров мне кажется вполне оправданным