Размышление об архитектуре класса/классов

Если не можешь следовать правилу «кто создает, тот и удаляет» (добавить в репу destroy(ID), destroy(A)), то придется городить костыли вроде твоих.

Я скоре сишник, чем плюсовик, но смотри:
Почему бы тебе не мучаться со смарт-поинтерами ежели у тебя есть repository?
Пусть он хранит всю информацию об А, а другие классы получают/освобождают объекты лишь с помощью методов repository.
Лишняя нагрузка невелика, зато всё просто и централизованно.

Далее нам нужно как то научить A (в деструкторе), обращатся к repository и удалять себя из него

Зачем? У shared_ptr есть специальный костыль для этого - custom deleter. Вот путь этот кастомный deleter сигналит репозиторию, что объект скончался.

У меня когда-то была похожая задача, но поскольку всего пар у меня в repository map было ограниченое количество (т.е. их видов было много, но в процессе работы программы их было не больше штук трёх, в зависимости от конфигурации), то я решил просто не удалять weak_ptr. Способа легко удалять weak_ptr из repository map не придумал.

чёртчёртчёрт. Ненавижу спп.

'A' это базовый класс, у него есть наследники. Так просто проще, в A сделал, наследники сразу получили.

У меня в repository на протяжении жизни постоянно будут вставлять и удалять. Так что нужно подчищать за собой.

'A' это базовый класс, у него есть наследники

И какая хрен разница? «A» может быть хоть чем, он в процессе передачи сообщения из deleter'а вообще не участвует. Deleter просто вызывает что-нибуть типа repo.destroy(ID).

Сейчас я в методе make написал толстый комментарий на весьма стремную логику.

Само собой метод make(ID) работает захватив мютекс. Теперь я прадставлю вашему вниманию метод release(ID). Он тоже работает захватив мютексов. Он является функтором который храниться в A и вызывается при удалении A.

Вопрос, безопасенли метод make(ID) если мы знаем что в нем есть логика weak_ptr to shared_ptr?

Ответ - может быть. И вот почему.

Вот так наверно выглядил бы метод make(ID)

shared_ptr<A> make(ID id){
    shared_ptr<A> holder(...);
    weak_ptr<A> wptr(holder);
    
    lock_guard();
    pair<...> res = storage.insert(make_pair(id, wptr));
    
    return wptr.lock();
}

И так, мы создали объект A, на случай если его небыло в storage. После операции insert в res.first у нас итератор либо на только что созданный wptr (который мы положили в storage) либо на тот который уже там лежал. Все по честному. Далее мы из wptr получаем shared_ptr и возвращаем его.

А теперь представьте что в момент между insert() и wptr.lock() (при условии что в storage уже лежал weak_ptr<A> для id) объект A был удален (точнее сказать он начал удалятся) и заблокировался на вызове release(ID) который встал на мютексе. Вот этот кейс и пришлось мне комментировать ибо прям магия,черная :) Попробуем сделать вот так

...
    shared_ptr<A> ptr(wptr.lock());
    if(ptr)
        return ptr;
        
    storage.remove(id);
    storage.insert(make_pair(id, wptr));
    return holder; 

Опять проблема. Как только мы отпустим mutex, метод release(ID) почистит объект который мы только что положили. Блин, а как предотвратить это ячет и не знаю :(

Да тебе уже подсказали всё что нужно. Вставь какую-нибудь лямбду в удалятор, которая и делит делает и из списка удаляет.

Чудесное решение!

Упс, забыл сказать, я не работаю в shared_ptr<A> напрямую. У меня pimpl.

class A \\ <- Этот класс базовый, наследуется реализация
{
class impl{} // <- Это виртуальный класс, наследники перегружают методы
shared_ptr<impl>
}

я не работаю в shared_ptr<A> напрямую

Я че-то не понимаю, а это тогда что:

shared_ptr<A> make(ID id){
    shared_ptr<A> holder(...);
    weak_ptr<A> wptr(holder);
    
    lock_guard();
    pair<...> res = storage.insert(make_pair(id, wptr));
    
    return wptr.lock();
}

Значит, всё-таки работаешь?

А зачем pimpl в виде shared_ptr? Отдаёшь его наружу?

Да сделай в make поиск, если нет - создал, заинсертил, вернул; если есть - залочил, вернул. И всё это с мутексом. А в делетере удаляй нафиг всё, и тоже с мутексом.

Что-то вроде:

std::shared_ptr <A> make(ID id) {
  ScopedLock l(mutex);
  auto it = map->find(id);
  if (it == map->end()) {
    std::weak_ptr <A> w;
    std::shared_ptr <A> s(new A, [&, =id] (A *p) {
      ScopedLock l(mutex);
      auto it = map->find(id);
      map->erase(it);
      delete p;
    });
    w = s;
    map->insert({id, w});
    return s;
  } else {
    return it->second->lock();
  }
  
}

Опять проблема. Как только мы отпустим mutex, метод release(ID) почистит объект который мы только что положили

Тебе нужно обеспечить, чтобы make/release работали бы только из одного треда. Для этого нужен вспомогательный класс, через объект которого будут вызываться make/release. На время существования такого объекта доступ к make/release лочится.

Что-то вроде этого

{
  MakeReleaseLock mr(repo.getMakeReleaseLock()); // получили интерфейс make/release и залочили его

  A a=mr.make(123);
  // release не может быть вызван из другого треда

} // Освобождается лок, теперь другой тред может разлочится и получить интерфейс make/release

Так написано для простоты.

Ваш пример делает ровто тоже что и мой, и не решает возникающую проблему. Между

auto it = map->find(id);

} else {
    return it->second->lock();
  }

Может быть вызван деструктор А (или функтор переданный в shared_ptr). В этот момент lock() уже не может из weak_ptr сделать shared_ptr, если был вызван деструктор объект (не важно выполнился ли он или только начал) объекта уже фактичеки нет и weak_ptr знает это и он вернет пустой shared_ptr.

Тебе нужно обеспечить, чтобы make/release работали бы только из одного треда

эт наврядли получится сделать. Да и ваш пример делает не совсем это. Какая разница где я захватываю мютекс: по отдельности в make и release или в MakeReleaseLock а потом уже вызываю make и release но без мютексов.

Представьте, что два треда (один делает make, второй release) делают

MakeReleaseLock mr(repo.getMakeReleaseLock());

Или я вас не так понял?

Можно вообще не подчищать. Можно подчищать, но из разных точек программы: из деструктора shared_ptr, при новом вызове make, или вообще по таймеру. В зависимости от предполагаемой нагрузки на repository, времени жизни каждого ID и соотношения числа «живых» записей к «мёртвым» каждый способ может иметь смысл. Надо сначала это решить, а потом уже думать, как реализовать.

Хм, решение вроде бы было простым, в remove(ID) удалять weak_ptr только если он expired().

Хорошее предложение. Правда хоть какойто вариант надо сделать сейчас, а потом когда начнем использовать, уже можно помсотреть статистикику.

И они оба хотят работать с id=123. Возникает ровто таже проблема, что я описал выше.

Вроде бы не возникает, или я вас неправильно понял. В моём примере make и release могут вызываться только синхронно, т.е. из одного треда, последовательно.

Представьте, что два треда (один делает make, второй release) делают

MakeReleaseLock mr(repo.getMakeReleaseLock());

Один из них получит объект для работы, а второй тред залочится. Соответственно пока первый тред не отработает, из второго треда release вызвать нельзя, вмешаться в работу make нельзя. Значит make может сделать все, что положено, вернуть правильный указатель и т.д.

Может быть вызван деструктор А

Не может, потому что scopedlock. Смотри внимательнее.

синхронно, т.е. из одного треда, последовательно.

Угу, но не из одного треда, и именно последовательно. Но проблема была не в том как синхронизовать вызовы, а в том что бы relase(123) не удалял ничего если мы в make(123) только что положили объект и он еще используется. Т.е. по сути relese был вызван для одного объекта, а пока он ждал на мютексе, объект в очереди уже поменяли, вот только у него остался тот же ID. И это нормально, что так происходит. Читайте выше, мне кажется что в release достаточно проверять weak_ptr на expired. Правда нас это обязывает вызывать release только после того как объект фактически был (или начал) удалятся. Но меня это более чем устраивает, так как именно это и нужно.

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

Правильно. А тот, кто его будет искать создаст новый и вставит его в таблицу уже после того, как.

А может и перед. Откуда нам знать кого система первым пустит.

что бы relase(123) не удалял ничего если мы в make(123) только что положили объект и он еще используется. Т.е.по сути relese был вызван для одного объекта, а пока он ждал на мютексе, объект в очереди уже поменяли, вот только у него остался тот же ID.

В моем примере это как раз и невозможно, т.к. во время вызова make никто не может вызвать release. Другие треды лочатся до того, как смогут делать вызовы make или release.

не в том как синхронизовать вызовы, а в том что бы relase(123) не удалял ничего если мы в make(123) только что положили объект и он еще используется

малость не понял. Раз вызывается release(123), то объект 123 какбы уже существует в этот момент, и make(123) должен вернуть именно его, а не создавать новый. Не так?

Да ктож им помешешает? Мютекс? Он лишь приостановит их. Дело же не в том, что вот непосредственно release был вызван, а то с каким параметром он был вызван. Я же немогу вызвать release если я не знаю еще значения параметра. А если я его уже знаю, то какая разница, что я остановился на мютексе?

Тред 1: Вот я хочу удалить объект для id = 123. Потому что им уже никто больше не пользуется, и я последний кто им пользовался. Как я об этом узнал? Да потому что я деструктор объекта и меня вызвали или вот прям уже щас вызовут.

Тред 2: Я хочу начать пользоваться объектом, мой id = 123. Я получил зеленый свет.

Тред 1: черт, меня приостановила система на мютексе. Но я то уже думал что я никому не нужен поэтому я обнулил в shared_ptr все счетчики.

Тред 2: Я лезу в очередь, нахожу объект с id = 123, вернее слабую ссылку на него (которая уже мертвая по сути). Я такой распрекрасный, вижу что ссылка мертвая, создаю новый объект, кладу его вместо старого и завершаю работу.

Тред 1: наконецтаки меня пропустили. Так где там объект с id = 123. Ты? Ок! Пошел вон отседова.

Возмущенный голос из далека: какого хера вы его удалили?

А что бы этого не получилось, Тред 1 перед удаление, должен всетаки убедится что weak_ptr уже expired.

Просто кроме как в деструкторе объекта, я больше ни как не могу узнать когда же мне нужно вызвывать release для конкретного ID.

Как я об этом узнал?

Действительно, как? Ну взял, попользовался, теперь освободить надо. Последний - не последний, я не знаю, это не моё дело, пусть repo думает, а я просто закажу вызов release.

Тред 2: Я хочу начать пользоваться объектом, мой id = 123. Я получил зеленый свет.

Тред 1: Черт, меня приостановила система на мютексе. Ну что ж, ждём.

Тред 2: Я заказываю объект 123. repo говорит: есть такой, бери.

Тред 2: Взял 123, теперь можешь пропустить остальных.

Тред 1: Мой заказ наконец-то исполняют. Что там repo решит - не моё дело. Я отстрелялся.

repo: Я дал 123 другому заказчику. Ок, ничего не делаем, пусть живёт.

Раз вызывается release(123), то объект 123 какбы уже существует в этот момент, и make(123) должен вернуть именно его, а не создавать новый. Не так?

Да так. Но не release(123) приводит к удалению объекта. А удалене объекта вызывает release(123), что бы подчистить всех хвосты на себя.

Да, я понял о чём ты.

Ну тогда делай актор, который хранит список этих самых А с последовательной обработкой сообщений и шли ему «Дай мне А с таким ID» и «Мне больше А с таким ID не нужен» и считай ссылки сам. Такое точно будет работать. Только реализовать сложнее.

repo: я дал 123 другому заказчику. Ок, ничего не делаем, пусть живёт.

А как он узнает что он кому то его дал? Или что уже никто им не пользуется и вот именно на этот раз удалить его понастоящему?

Значит, мне нужен счетчик внутри repo для каждой записи, значит нужно считать число make/release (на что то это похоже, да?). Это конечно сработает, но ведь не зряже народ ушел от простых указателе new/delete к умным? Я вот точно не хочу делать ручной подсчет make/release.

не зряже народ ушел от простых указателе new/delete к умным?

И в чем проблема использовать умный указатель для подсчёта make/release?

Ты пытаешься изобрести заново garbage collector, а на самом деле тебе нужен шаблон Фабрика.

Храни внутри weak_ptr, а наружу отдавай shared_ptr. Когда все внешние шареды удалятся, внутренний ptr обнулится автоматически. Тогда в make(), кроме проверки существования указателя, нужно ещё добавить проверки его валидности. И всё.

Вы прям в точку, а я все ни как не мог понять на что это похоже.

в make(), кроме проверки существования указателя, нужно ещё добавить проверки его валидности

диапозон ID неприлично большой. А вот повторность запроса ID вообще не определена. Так что можно плучить ситуацию... ну в общем то да, garbage collector со всеми его проблемами :(

какой-то в топике поток мыслей сбивающий с толку :)

давайте по шагам, разберёмся с тем что-же на самом деле спросил автор :-) Если я его правильно понял то:

1) есть некий repository хранящий (возможно долговременно и персистентно) объекты разных классов

2) объекты идентифицируются по ID, что логично

3) и кто-то, с пьяну либо по укурке, сделал в repository метод make(ID &)..выбор имени уже как-бэ намекает :) что кого-то там надо уволить

4) а так как плюсам похрен до возвращаемого значения, метод един на все типы и отдаёт он местный аналог void *, то есть нечисть кастуемую куда угодно

автору досталась задача - известить repository о том что выданный ранее объект произвольного класса более не нужен

Читайте Размышление об архитектуре класса/классов (комментарий), он вполне ясно характеризовал задачу, а из нее уже понятно какие вытекают проблемы.

объекты разных классов

Разных, но из одной иерархии.