Problem mit ungültigem Iterator

KN4CK3R

Hallo,

ich habe ein Event-System, bei dem der Benutzer bei verschiedenen Events EventHandler registrieren und entfernen kann. Die EventHandler werden beim Event in einer std::list verwaltet. Wenn ein Event ausgelöst wird, gehe ich die Liste von vorne nach hinten durch und rufe die im EventHandler enthaltene std::function auf. Dabei kann es passieren, dass innerhalb der aufgerufenen Funktion, der gleiche EventHandler aus der Liste gelöscht wird. Problem an der Sache ist, dass sobald die std::function zurückkehrt, der Iterator der aufrufenden Schleife nicht mehr gültig ist. Ich habe mir dazu ein paar Problemlösungen überlegt, die ich aber alle nicht sonderlich toll finde. Deswegen wollte ich fragen, ob hier schonmal jemand ein ähnliches Problem hatte und wie es dort gelöst wurde.

Pseudocode:

class EventHandler
{
	std::function<void()> handler;
};

class Event
{
	std::list<EventHandler> eventHandler;

	void Add(EventHandler eh)
	{
		eventHandler.push_back(eh);
	}

	void Remove(EventHandler eh)
	{
		eventHandler.remove(eh);
	}

	void Invoke()
	{
		for (auto it = eventHandler.begin(); it != eventHandler.end(); ++it)
			(*it).handler();
	}
};

EventHandler eh([]()
{
	event.Remove(eh);
});
event.Add(eh);
event.Invoke();

Lösung 1: statt std::list std::vector verwenden und nach einem Funktionsaufruf per size() prüfen, ob sich etwas geändert hat. Beim Vector kann man dann die Zählvariable einfach gleich lassen.
Lösung 2: der EventHandler Klasse ein remove Flag hinzufügen und vor dem Aufrufen prüfen, dass dieses Flag nicht gesetzt ist und nach dem Aufruf prüfen, ob es gesetzt ist und falls ja den EventHandler in der Invoke-Schleife entfernen
Lösung 3: ähnlich wie 2 nur eine weitere Löschliste einführen und dort die zu löschenden EventHandler eintragen. Nach der Invokeschleife diese dann löschen. Problem: vor jedem Aufruf muss geschaut werden, ob der EventHandler in der Löschliste steht und falls ja, nicht mehr aufrufen. Ist zwar sicherlich nicht performancelastig, da die Liste sicher nicht größer wird wie 1-2 Einträge.

Weitere Lösungsmöglichkeiten?

greetz KN4CK3R

Kleiner Tipp:

In der Schleife in #23,nimm den Pfeiloperator.

314159265358979

Hacker schrieb:

Kleiner Tipp:

In der Schleife in #23,nimm den Pfeiloperator.

Was für ein, für die Problemstellung absolut unrelevanter, "Tipp".

@TE: Ich würde einen vector an Zeigern auf die zu löschenden Handler anlegen und die Handler dann über die Zeiger nach dem Iterieren löschen.

Was ich aber nicht verstehe: Warum lässt du den Benutzer nicht gleich eine std::function als EventHandler verwenden? Warum muss er die EventHandler-Klasse verwenden?

KN4CK3R

fürs Löschen muss ich die std::functions vergleichen können und std::function lassen sich wohl nicht vergleichen (siehe diverse Threads im Internet). Die EventHandler Klasse beeinhaltet noch eine fortlaufende ID über die ich den Vergleich mache.

Edit:
http://stackoverflow.com/questions/3629835/why-is-stdfunction-not-equality-comparable

Edit2: ich merke gerade .NET macht das wohl auch so:

private void button1_Click(object sender, EventArgs e)
{
	button1.Click -= ev; // = button2_Click
}

private void button2_Click(object sender, EventArgs e)
{

}

Beide Funktionen werden beim ersten Klicken ausgeführt, danach nur noch eine. Eigentlich aber nicht intuitiv die Lösung? Wenn ich einen EventHandler entferne, sollte er nicht nochmal aufgerufen werden können danach. Naja egal, dann mach ichs mit ner Löschlist/vector

greetz KN4CK3R

Skym0sh0

du könntest es auch so machen, dass du vor dem funktiosn aufruf dir einen temporären iterator erzeugst und den schon eine position weiter gehen lässt. und danach inkrementierst du deinen schleifen iterator nicht sondern weist ihm den temporären zu

for (auto it = eventHandler.begin(); it != eventHandler.end(); ++it)
{
    auto tmpIt = it++;
    it->handler();
    it = tmpIt;
}

iteratoren sind ja nicht die grössten und komplexesten objekte, von daher wird das auch nicht schlimm sein, die zuweisung immer zu machen anstatt einer einfachen inkrementierung

KN4CK3R

ohne es jetzt ausprobiert zu haben, der Code scheint Fehler zu enthalten.
Meiner Meinung nach müsste das so aussehen oder?

for (auto it = eventHandler.begin(); it != eventHandler.end(); )
{
    auto tmpIt = it;
    it++;
    tmpIt->handler();
}

Edit: danke für den Hinweis, die Methode sieht am besten aus
http://ideone.com/V7cpn

greetz KN4CK3R

Skym0sh0

ups, ja stimmt jetzt wo dus sagst xD

aber du hast erfasst was ich meinte, und mehr als ein beispiel sollte es nicht sein

Tachyon

Kann es sein, dass Du sowas wie boost::signals nachbaust?

DocShoe

Ich hab´ sowas Ähnliches gebaut, allerdings lege ich nicht die Funktionszeiger in einem Vektor ab, sondern sowas:

struct EventCallback
{
   bool Removed;
   FuncPtr Callback;
};

Während des "Feuerns" des Events wird der entsprechende Eintrag nicht aus dem Vektor entfernt, sondern nur das Removed Flag gesetzt. Am Ende der Funktion werden dann alle Einträge entfernt, deren Removed Flag gesetzt ist.
Die Funktion zum Austragen eines Callbacks arbeitet in zwei Modi: Wenn sie aufgerufen wird, während Events abgefeuert werden, markiert sie den entsprechenden Eintrag zum Löschen, ansonsten entfernt sie ihn aus dem Vektor.
Zuletzt musst du vorm Aufrufen der Callbacks noch prüfen, ob das Removed Flag gesetzt ist.

KN4CK3R

@Tachyon: boost::signals kenne ich nur vom Namen her, wie das dort gemacht wird, weiß ich nicht

@DocShoe: das wäre meine Möglichkeit 2 gewesen, die Lösung von Skym0sh0 gefällt mir allerdings besser, weil sie ohne solch ein Flag auskommt.

greetz KN4CK3R

KN4CK3R

ich hab das jetzt einmal testen können und musste feststellen, dass die Version doch nicht so gut ist, wie sie scheint.
Wenn das Event entfernt wird, auf das man it in der Zeile auto tmpit = it++ zeigen lässt, dann landet man wieder beim Anfangsproblem. :|
Also dann wohl doch DocShoe's Lösungsansatz.

greetz KN4CK3R

camper

KN4CK3R schrieb:

ohne es jetzt ausprobiert zu haben, der Code scheint Fehler zu enthalten.
Meiner Meinung nach müsste das so aussehen oder?
for (auto it = eventHandler.begin(); it != eventHandler.end(); )
{
    auto tmpIt = it;
    it++;
    tmpIt->handler();
}
Edit: danke für den Hinweis, die Methode sieht am besten aus
http://ideone.com/V7cpn

greetz KN4CK3R

Setzt aber gültige Iteratoren nach dem erase voraus, funktioniert folglich nur mit Listen. Besser ist es, den Rückgabewert von erase zu verwenden.

for (auto it = l.begin(); it != l.end();)
        {
                cout << *it << endl;
                x++ % 2 == 0 ? it = l.erase(it) : ++it;
        }

DocShoe

Das Problem ist, dass man beim Absetzen der Events keine Informationen darüber bekommt, dass sich jemand aus der Liste austrägt. Damit verlierst du den Rückgabewert von erase und kannst ihn nicht benutzen.

KN4CK3R

habs jetzt so gemacht:

//die Remove Funktion setzt remove auf true
for (auto it = eventHandler.begin(); it != eventHandler.end();)
{
	EventHandler &info = *it;
	if (!info.remove)
	{
		info.eventHandler.GetFunction()(std::forward<T>(param)); //fire event
	}
	if (info.remove)
	{
		it = eventHandler.erase(it);
	}
	else
	{
		++it;
	}
}

Dieser Version sollte failsafe sein auch wenn im ersten EventHandler alle anderen gelöscht werden würden. Auch das hinzufügen von neuen EventHandlern ist so abgedeckt.

greetz KN4CK3R