Discussion :

[Bktero]

Bonjour,

J'ai une classe qui joue un rôle d'observateur et qui possède une liste d'observables (pour pouvoir être notifié par plusieurs sources). Dans le destructeur de cette classe, je parcours la liste et pour chaque observable, je demande à ce que mon objet soit retiré de la liste des observateurs. Le code est comme ceci :

(observateur)

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
MultipleDefaultObserver::~MultipleDefaultObserver() {
	for (auto& detector : detectors_m) {
		if (detector != nullptr) {
			detector->removeObserver(*this);
		}
	}
}

(observable)

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
void DefaultDetector::removeObserver(DefaultObserver& observer) {
	for (auto& slot : observers_m) {
		if (slot == &observer) {
			slot = nullptr;
			break;
		}
	}
}

Comme je ne fais pas d'allocation dynamique, detectors_m et observers_m sont des std::array de pointeurs.

J'ai écris ce code, donc il me parait OK, sinon j'aurais fait autrement. Valgrind n'est pas de mon avis et me signalent des invalid read/write of size 4 dans le destructeur. En commentant la ligne detector->removeObserver(*this);, les erreurs disparaissent. Un exemple de message de Valgrind :

==1687== Invalid read of size 4
==1687== at 0x12E5A9: supervisor::DefaultDetector::removeObserver(supervisor::DefaultObserver&) (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x12E917: supervisor::MultipleDefaultObserver::~MultipleDefaultObserver() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x26A6F3: ____C_A_T_C_H____T_E_S_T____8() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x181BD2: Catch::RunContext::invokeActiveTestCase() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1933D0: Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1A4FF0: Catch::RunContext::runTest(Catch::TestCase const&) (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1A58FC: Catch::(anonymous namespace)::runTests(std::shared_ptr<Catch::Config> const&) (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1AA01D: Catch::Session::runInternal() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1AA160: Catch::Session::run() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x114FD3: main (in /media/sf_shared/Common/cmake-debug/common)
==1687== Address 0x4dcfb90 is 0 bytes inside a block of size 440 free'd
==1687== at 0x48318D7: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==1687== by 0x26A6D7: ____C_A_T_C_H____T_E_S_T____8() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x181BD2: Catch::RunContext::invokeActiveTestCase() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1933D0: Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1A4FF0: Catch::RunContext::runTest(Catch::TestCase const&) (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1A58FC: Catch::(anonymous namespace)::runTests(std::shared_ptr<Catch::Config> const&) (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1AA01D: Catch::Session::runInternal() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1AA160: Catch::Session::run() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x114FD3: main (in /media/sf_shared/Common/cmake-debug/common)
==1687== Block was alloc'd at
==1687== at 0x483087B: operator new(unsigned int) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==1687== by 0x26A458: ____C_A_T_C_H____T_E_S_T____8() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x181BD2: Catch::RunContext::invokeActiveTestCase() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1933D0: Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1A4FF0: Catch::RunContext::runTest(Catch::TestCase const&) (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1A58FC: Catch::(anonymous namespace)::runTests(std::shared_ptr<Catch::Config> const&) (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1AA01D: Catch::Session::runInternal() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x1AA160: Catch::Session::run() (in /media/sf_shared/Common/cmake-debug/common)
==1687== by 0x114FD3: main (in /media/sf_shared/Common/cmake-debug/common)

Vu que j'utilise un auto-range for, je ne pense pas dépasser des tableaux. La seule hypothèse qui me vient à l'esprit est que *this n'est plus une référence valide à ce moment là. Vous pouvez confirmer ? Comment faire pour que ça fonctionne correctement ?

Merci d'avance !

[Bousk]

Pour moi il n'y a pas de probleme dans ce code. this est encore valide dans le destructeur, *this l'est tout autant.

[Piraaate]

Si MultipleDefaultObserver est une classe mère, peut-être qu'il considère qu'une fois dans le destructeur de celle-ci, une partie de *this n'est plus valide (destruction des membres dérivés) et rapporte une erreur alors que effectivement l'adresse est valide ?

[Bktero]

Effectivement, dans mon code de test, MultipleDefaultObserver est une classe mère. Les erreurs se produisent lors de la destruction d'instance de MyMultipleDefaultObserver, une implémentation faite pour les tests unitaires.

J'ai donc essayé de créer une méthode protected nommée stop() dans laquelle j'ai extrait le boulot du destructeur. Ainsi, le destructeur de MultipleDefaultObserver est déclaré = default et le destructeur de MyMultipleDefaultObserver appelle stop().

J'ai toujours les mêmes erreurs

C'était une très bonne idée en tout cas

[Bktero]

C'est bon j'ai trouvé !

Le problème n'est pas (vraiment) dans la classe mais dans son utilisation. En fait, je me suis rendu compte que seul deux de mes test cases généraient des erreurs avec Valgrind. En voici un :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
TEST_CASE("It is possible to know how many detectors are currently observed", "[supervisor][MultipleDefaultObserver]") {
	MyMultipleDefaultObserver observer;
 
	CHECK(observer.getDetectorCount() == 0);
 
	auto maxCount = observer.getMaxDetectorCount();
	std::vector<MyDefaultDetector> detectors(maxCount);
 
	for (std::size_t i = 0; i < maxCount; ++i) {
		observer.observe(detectors[i]);
		CHECK(observer.getDetectorCount() == i + 1);
	}
}

Le problème est que le vecteur detectors est détruit avec observer.

Du coup j'ai créé un bloc à l'intérieur de la fonction : le vecteur est créé en dehors, l'observer à l'intérieur. Valgrind est content ; moi aussi mais pas tant que ça car je me dis que ce destructeur est à risque... Qu'en pensez-vous ?

[Piraaate]

Ok je comprends, j'avoue que dans ce genre de design pattern je préfère que seul un des deux (observateur ou observé) ait une liste de l'autre.
Cela oblige à gérer "explicitement" la liste, mais cela évite une gestion croisée qui rend le code plus difficile à lire/tracer ou encore des difficultés à l'utilisation (comme dans ton cas).