Destructor-Handling bei Exception aus einem Constructor

[hustbaer]

@großbuchstaben
Man kann, aber ich würde sagen dass das der komplett verkehrte Ansatz ist. Das Problem entsteht bloss durch das nicht-Aufteilen einer zu grossen Klasse in kleinere. Workarounds wie "aufräumen vor dem Werfen" verschlimmern die Sache bloss noch durch unnötige Komplexität. Mal ganz davon abgesehen dass sie total brüchig sind, weil jede kleine Änderung neue Stellen schaffen kann wo Exceptions fliegen könnten (irgendwo in einer Unterfunktion einer Unterfunktion die im Ctor aufgerufen wird) - was dann zu einem Fehler/Leak führt.

Und Workarounds dieser Art lenken den Blick vom eigentlichen Problem ab, nämlich davon dass man eine Klasse hat die für zu viel zuständig ist.

@shft
Es werden alle Objekte zerstört die vollständig konstruiert wurden.
D.h. Member werden zerstört, da die Member für sich ja vollständig konstruiert wurden. Ebenso werden vollständig konstruierte Basisklassen zerstört.

Die einfachste Lösung für das Problem: alles so weit runterbrechen dass jede Klasse nur mehr genau eine "Resource" kontrolliert.
Bzw. allgemeiner: genau eine Sache macht ("do") die der Konstruktor rückgängig machen muss ("undo").

Anstatt also z.B. zwei dynamisch angeforderte Arrays in einer Klasse X zu verwalten, bastelt man sich erstmal eine "dynamisches Array" Klasse A, die genau ein solches Array konstruiert.
Und verwendet dann zwei A Member in der Klasse X.
Bzw. wenn es um dynamische Arrays geht macht man das natürlich nicht, sondern verwendet gleich std::vector .

Bei anderen Dingen allerdings, wo es noch keine fertigen RAII Wrapper gibt, schreibt man sich diese einfach selbst. Wobei man ruhig den Ball flach halten kann. Je nach Anwendungszweck kann es vollkommen ausreichend sein eine struct mit lediglich einem public Datenmember und einem Destruktor zu haben. "Ist zu aufwendig" ist also keine Entschuldigung dafür es falsch zu machen. (Erwähne ich nur weil ich dieses "Gegenargument" schon öfter gehört habe.)

[großbuchstaben]

seine Frage war, wie man aufräumt, wenn im Konstruktor eine exception geworfen wird.

Meine Antwort ist: Räum' auf, bevor du die exception wirfst.

Klassendesign war hier ja nicht gefragt.

[amlog]

großbuchstaben schrieb:

seine Frage war, wie man aufräumt, wenn im Konstruktor eine exception geworfen wird.

Meine Antwort ist: Räum' auf, bevor du die exception wirfst.

Klassendesign war hier ja nicht gefragt.

Wenn jemand fragt, "wie mache ich am besten Selbstmord", gibst du ihm dann eine Anleitung oder versuchst du, ihn davon abzuhalten?

[hustbaer]

großbuchstaben schrieb:

Meine Antwort ist: Räum' auf, bevor du die exception wirfst.

Das hab' ich schon verstanden. Ist bloss die falsche Antwort.
Die richtige ist: gar nicht, denn es hat dort nichts aufzuräumen zu geben.

[TDDler]

hustbaer schrieb:

großbuchstaben schrieb:

Meine Antwort ist: Räum' auf, bevor du die exception wirfst.

Das hab' ich schon verstanden. Ist bloss die falsche Antwort.
Die richtige ist: gar nicht, denn es hat dort nichts aufzuräumen zu geben.

hustbaer hat recht. Wenn die Klasse vernünftig entworfen ist, dann ist der Rumpf eines Konstruktors LEER.

A::A(void)
: x()
, y(bla)
, z(bla, bla)
{
return;
}

Wirft z() eine Exeption, dann werden x und y automatisch richtig gelöscht und dabei selbstverständlich der Destruktor von x und y aufgerufen. Deswegen müssen x und y Objekte sein und nicht nur einfache Zeiger. Denn der "Destruktor" eines Zeigers löscht nur sich selbst, aber nicht das Ding auf das er zeigt. Ein std::unique_ptr() dagegen löscht auch das Ding auf das er zeigt. Wenn x() der Kontruktor der Basisklasse ist, y ein Zeiger und bla ist "new Irgendwas", dann bleibt ein Irgendwas-Objekt übrig, auf das niemand mehr zugreifen kann (= memory leak), wenn z() eine Ausnahme wirft. Deshalb macht man sowas nicht, sondern nur mit einem smart pointer. Ggf. schreibt man sich eine eigene kleine Klasse, wie hustbaer bereits geschrieben hat. Von der wird dann auch der Destruktor aufgerufen, sollte das Objekt bereits vollständig initialisiert worden sein. Nur gibt es in der STL bereits so viele gute Lösungen, dass das praktisch kaum noch vorkommt.

Alles klar? Good luck.

[stl]

TDDler schrieb:

Nur gibt es in der STL bereits so viele gute Lösungen, dass das praktisch kaum noch vorkommt.

STL != Standardlibrary.

[tntnet]

großbuchstaben schrieb:

seine Frage war, wie man aufräumt, wenn im Konstruktor eine exception geworfen wird.

Meine Antwort ist: Räum' auf, bevor du die exception wirfst.

Klassendesign war hier ja nicht gefragt.

Es ist durchaus sinnvoll, einen Schritt zurück zu gehen und schauen, warum die Frage überhaupt auftaucht. Häufig gibt es bessere Lösungen, die das Problem von vornherein vermeiden. Und das ist hier so ein Fall, wo es sich lohnt zu überlegen, wie man das Problem elegant vermeiden kann.

[großbuchstaben]

TDDler schrieb:

Wenn die Klasse vernünftig entworfen ist, dann ist der Rumpf eines Konstruktors LEER.

im Idealfall ja. In der Praxis wäre ich mit solchen Pauschalbehauptungen vorsichtiger, da gibt es öfters mal Randbedingungen und Vorgaben, die es nicht erlauben zu programmieren wie in example 4.3.2 deines C++ Lehrbuchs.

ändert aber auch nix am Sachverhalt: Klassendesign hin oder her, gefragt war nach dem Aufräumen bei exception im Konstruktor und meine Antwort ist "erst aufräumen, dann werfen" und fertig

Deswegen müssen x und y Objekte sein und nicht nur einfache Zeiger.

"müssen" muß gar nichts. Wenn ein langjähriges 100k+ LoC Projekt zeigerbasiert designt ist, und jemand finge in Hierarchieebene 6 plötzlich mit Referenzen oder smart pointers an ...

[Finnegan]

großbuchstaben schrieb:

im Idealfall ja. In der Praxis wäre ich mit solchen Pauschalbehauptungen vorsichtiger...

Gut, das du das ansprichst. Solche scheinbar in Stein gemeißelten Regeln wie "man darf nie...", "man muss immer..." liest mal leider zu häufig, und so wie sie formuliert sind, sind sie fast immer falsch. Korrekter sollte es eigentlich heißen "Man sollte bevorzugt..., weil..., es sei denn man hat einen guten Grund es anders zu machen". Das gilt für fast alle dieser No-Gos, manchmal (jedoch meistens nicht!) sind sie die beste und eleganteste Lösung für ein Problem.

Kürzlich habe ich sogar mal folgendes in Produktivcode geschrieben:

...
std::shared_ptr<Type>* ptr = new std::shared_ptr<Type>(other); 
...
delete ptr;
...

Und obwohl das vermutlich auf den ersten Blick viele entsetzt aufschreien lässt, tat ich das aus gutem Grund, und habe den Code ohne schlechtes Gewissen eingecheckt

Natürlich sollte man besonders Anfänger immer darauf hinweisen, dass man gewisse Dinge besser sein lässt, aber nicht ohne den Hinweis, dass es auch (seltene) Ausnahmen gibt (nicht dass sie sich nacher nicht trauen eine dieser goldenen Regeln zu verletzten, obwohl der Alternativcode undgleich unverständlicher, komplexer oder sogar unmöglich ist).

Finnegan

[proof by contradiction]

Finnegan schrieb:

Kürzlich habe ich sogar mal folgendes in Produktivcode geschrieben:

...
std::shared_ptr<Type>* ptr = new std::shared_ptr<Type>(other); 
...
delete ptr;
...

Und obwohl das vermutlich auf den ersten Blick viele entsetzt aufschreien lässt, tat ich das aus gutem Grund, und habe den Code ohne schlechtes Gewissen eingecheckt

Mehr Kontext bitte. Das kann so nicht stimmen.

[hustbaer]

großbuchstaben schrieb:

ändert aber auch nix am Sachverhalt: Klassendesign hin oder her, gefragt war nach dem Aufräumen bei exception im Konstruktor und meine Antwort ist "erst aufräumen, dann werfen" und fertig

Ja, und deine Antwort ist immer noch falsch.
Weil "erst aufräumen, dann werfen" in einem Programm mit Exceptions nix verloren hat. Weil es zu Problemen (=Bugs) führt. Weil es falsch ist.

[hustbaer]

..

[Finnegan]

proof by contradiction schrieb:

Finnegan schrieb:

...
std::shared_ptr<Type>* ptr = new std::shared_ptr<Type>(other); 
...
delete ptr;
...

Mehr Kontext bitte. Das kann so nicht stimmen.

ptr wird an eine C-API übergeben, die ich nicht modifizieren kann, und als Argument einen void* erwartet, der dann wiederum in einem asynchronen Aufruf (anderer Thread) als User-Parameter an eine von mir implementierte Callback-Funktion übergeben wird. Diese Funktion benötigt die Type-Instanz, und es ist erforderlich, dass diese Funktion im Kontext des Threads dieser C-API aufgerufen wird (es handelt sich um eine Operation auf einem von der C-Bibliothek erzeugten OpenGL-Kontext, auf den ich keinen direkten Zugriff habe, der aber innerhalb dieser Callback-Funktionen aktiviert ist).

Nun könnte es passieren, das die Type-Instanz gelöscht wird, bevor die C-Callback-Funktion (asynchron) aufgerufen wird (wann das passiert, ist von - ebenfalls asynchronen - Ereignissen in anderen Threads abhängig, an die der shared_ptr weitergereicht wird).

Der Pointer auf den Shared Pointer dient dazu, die Type-Instanz so lange am Leben zu erhalten, bis die C-Callback-Funktion aufgerufen wurde (diese mach abschließend auch das delete auf dem Pointer). Wegen des o.g. void*-Arguments kann ich dieser API eben keinen shared_ptr übergeben, und weil die Type-Instanz sterben kann, bevor die C-Callback aufgerufen wurde kann ich auch keinen Type* übergeben ohne Gefahr zu laufen, dass ich Operationen auf einem Dangling Pointer mache.

In Code, über den ich vollständige Kontrolle habe, wirst du bei mir solche Konstrukte sicher nicht finden, aber manchmal machen auf den ersten Blick wahnwitzige "Patterns" doch Sinn.

Finnegan

[Finnegan]

hustbaer schrieb:

Ja, und deine Antwort ist immer noch falsch.
Weil "erst aufräumen, dann werfen" in einem Programm mit Exceptions nix verloren hat. Weil es zu Problemen (=Bugs) führt. Weil es falsch ist.

Vielleicht liegt ein Missverständnis vor, was hier verschiedene Leute unter "falsch" verstehen. Für mich ist ein Programm "falsch" wenn es nicht das korrekte Ergebnis liefert (UB oder nicht korrekter Algorithmus) oder wenn es syntaktisch falsch ist.

Recht gebe ich dir dahingehend, dass es so "hochgradig nicht empfehlenswert" ist, oder "mit höherer Wahrscheinlichkeit zu Fehlern führt". Natürlich sollte man möglichst vermeiden in Konstruktoren Exceptions zu behandeln, aber manchmal hat man keine andere Wahl, und dann (und nur dann!) sollte man eben aufräumen und dann werfen.

Finnegan

[proof by contradiction]

Würde ich trotzdem so schreiben:

auto ptr = std::make_unique<std::shared_ptr<Type>>(other); // oder sogar std::move(other)
... // hier könnte immer Code eingefügt werden, der wirft
c_api_with_callback(ptr.release());

Dein Code mag zwar in der Situation korrekt sein, beruht aber auf Subtilitäten (noexcept zwischen new und Callback-Aufruf) und geht deshalb sehr leicht kaputt. Deshalb lieber garantiert ausnahmensicher programmieren.

[Finnegan]

proof by contradiction schrieb:

Würde ich trotzdem so schreiben:

auto ptr = std::make_unique<std::shared_ptr<Type>>(other); // oder sogar std::move(other)
... // hier könnte immer Code eingefügt werden, der wirft
c_api_with_callback(ptr.release());

Dein Code mag zwar in der Situation korrekt sein, beruht aber auf Subtilitäten (noexcept zwischen new und Callback-Aufruf) und geht deshalb sehr leicht kaputt. Deshalb lieber garantiert ausnahmensicher programmieren.

Wenn dort Code steht, der werfen kann ja, allerdings wird der Shared-PointerPointer ausschliesslich für diesen einen Aufruf der C-API erzeugt. Die beiden Zeilen oben würden im Code direkt untereinander stehen, und daher in diesem Fall eklatant das "keep it simple, stupid!"- und das "programmier' nicht für Code, der hier irgendwann vielleicht mal stehen könnte"-Prinzip verletzen.

Generell ist dein Hinweis jedoch sinnvoll, aber nur wenn dazwischen auch tatsächlich Code steht (Funktionsaufrufe) bei dem man sich nie ganz sicher sein kann

Finnegan

[Nathan]

Finnegan schrieb:

Wenn dort Code steht, der werfen kann ja, allerdings wird der Shared-PointerPointer ausschliesslich für diesen einen Aufruf der C-API erzeugt. Die beiden Zeilen oben würden im Code direkt untereinander stehen, und daher in diesem Fall eklatant das "keep it simple, stupid!"- und das "programmier' nicht für Code, der hier irgendwann vielleicht mal stehen könnte"-Prinzip verletzen.

Letzteres ist aber ein deutlich wichtigeres Prinzip. Programming in the future sense. Du weißt nie, wie lange dein Code noch lebt. Irgendwann könnte ein anderer Programmierer da Code einfügen, der wirft, weil er schnell etwas fixen muss und/oder nicht auf Exceptionsafety achtet. Man muss so programmierern, das solche Fehler vermieden werden.
Darüber hinaus verletzt gezeigter Code jetzt auch nicht gerade KISS.

[hustbaer]

@Finnegan
Mir geht es dabei eher darum wie solcher Code überhaupt entsteht.

Das erste was einem dabei auffallen sollte, wenn man sowas schreibt, ist, dass es hier ein Wartbarkeitsproblem gibt.
Und zwar dass man bei Code dieser Art immer aufpassen muss dass eine Änderung nicht zu einem Problem führt.
Und wenn man das mal verstanden hat, dann sollte man sich die Frage stellen: wie kann man das verhindern?
Die Antwort wurde ja schon gegeben.

Und wenn man sich diese Dinge mal überlegt hat, dann sollte man mMn. solchen Code auch nicht mehr schreiben. Weil man schliesslich bereits weiss dass es problematisch ist, und wie man es besser macht.

Konstrukte dieser Art können also durchaus "richtig" sein, in dem Sinn dass ein Programm, so wie es ist, ohne weitere Änderungen, in keinem Fall UB sein wird, und in keinem Fall andere unerwünschte Dinge tun wird (z.B. Leaks, generell nicht das machen was das Programm machen sollte).
Sie deuten mMn. aber immer darauf hin dass der Programmierer etwas nicht verstanden hat oder es bewusst ignoriert.
Wenn du willst kannst du also sagen: der Code ist strenggenommen korrekt, aber der Programmierer ist defekt.

@proof by contradiction

proof by contradiction schrieb:

Würde ich trotzdem so schreiben:

auto ptr = std::make_unique<std::shared_ptr<Type>>(other); // oder sogar std::move(other)
... // hier könnte immer Code eingefügt werden, der wirft
c_api_with_callback(ptr.release());

Dein Code mag zwar in der Situation korrekt sein, beruht aber auf Subtilitäten (noexcept zwischen new und Callback-Aufruf) und geht deshalb sehr leicht kaputt. Deshalb lieber garantiert ausnahmensicher programmieren.

Wenn schon, dann so:

auto ptr = std::make_unique<std::shared_ptr<Type>>(other);
... // hier könnte immer Code eingefügt werden, der wirft
c_api_with_callback(ptr.get()); // ... und jetzt darf auch c_api_with_callback werfen wenn es mag
ptr.release();

Sonst gibt es nämlich ein Problem wenn die "C API" nicht wirklich eine "C API" ist, sondern C++ Code der hinter einer C-artigen Schnittstelle versteckt wurde.

[Finnegan]

Nathan schrieb:

Letzteres ist aber ein deutlich wichtigeres Prinzip. Programming in the future sense. Du weißt nie, wie lange dein Code noch lebt. Irgendwann könnte ein anderer Programmierer da Code einfügen, der wirft, weil er schnell etwas fixen muss und/oder nicht auf Exceptionsafety achtet. Man muss so programmierern, das solche Fehler vermieden werden.

Ja, das stimmt auch. Dieser kurze Code ist auch eigentlich ein schlechtes Beispiel dafür, was ich hier eigentlich vermeiden möchte: Dass Code Stück für Stück komplizierter wird und damit schwerer zu lesen und zu warten wird. Wenn man alle zukünftigen Eventualitäten berücksichtigt, kann sowas durchaus auch passieren. Schwer zu beurteilen, wo man da die Grenze zieht, und darüber kann man sicher lange diskutieren. Einen unique_ptr zu erstellen, der direkt in der nächsten Zeile seine Besitzansprüche aufgibt kommt mir etwas übertrieben vor - ich bevorzuge da etwas weniger Rauschen im Code, auch wenn ich ebenfalls Verfechter von Code bin, der hilft, zukünftige Fehler zu vermeiden (wie gesagt, ist alles etwas zweischneidig).

Nathan schrieb:

Darüber hinaus verletzt gezeigter Code jetzt auch nicht gerade KISS.

Wie gesagt, nach meinem Bauchgefühl schon, weil es ein kleines bisschen unnötige Komplexität reinbringt durch den zusätzlichen unique_ptr . Vielleicht wäre es besser das new in den Funktionsaufruf zu packen:

c_api_with_callback(new std::shared_ptr<Type>(other));

... und so deutlich zu machen "es ist nicht geplant, das hier etwas dazwischen steht", und " c_api_with_callback trägt jetzt die Verantwortung für den Speicher".

Finnegan

[Finnegan]

hustbaer schrieb:

Und wenn man sich diese Dinge mal überlegt hat, dann sollte man mMn. solchen Code auch nicht mehr schreiben. Weil man schliesslich bereits weiss dass es problematisch ist, und wie man es besser macht.

Wie bereits gesagt, da stimme ich generell zu! Ich habe lediglich ein Problem mit "immer" und "nie" - lieber "fast immer" und "fast nie", da es fast immer Ausnahmefälle gibt, und sei es nur für die handvoll Leute, welche die C++-Standardbibliothek programmieren, damit man überhaupt erstmal das Handwerkszeug hat, um auf solche Konstrukte verzichten zu können

Finnegan