Wie verhindern Sie, dass sich IDisposable auf alle Ihre Klassen ausbreitet?

138

Beginnen Sie mit diesen einfachen Klassen ...

Angenommen, ich habe eine einfache Reihe solcher Klassen:

class Bus
{
    Driver busDriver = new Driver();
}

class Driver
{
    Shoe[] shoes = { new Shoe(), new Shoe() };
}

class Shoe
{
    Shoelace lace = new Shoelace();
}

class Shoelace
{
    bool tied = false;
}

A Bushat a Driver, das Driverhat zwei Shoes, jeder Shoehat a Shoelace. Alles sehr dumm.

Fügen Sie Shoelace ein IDisposable-Objekt hinzu

Später entscheide ich, dass eine Operation auf dem ShoelaceMultithreading ausgeführt werden kann, also füge ich eine hinzu, EventWaitHandlemit der die Threads kommunizieren können. So Shoelacesieht es jetzt aus:

class Shoelace
{
    private AutoResetEvent waitHandle = new AutoResetEvent(false);
    bool tied = false;
    // ... other stuff ..
}

Implementieren Sie IDisposable auf Shoelace

Jetzt beschwert sich Microsoft FxCop jedoch: "Implementieren Sie IDisposable auf 'Shoelace', da Mitglieder der folgenden IDisposable-Typen erstellt werden: 'EventWaitHandle'."

Okay, ich implementiere IDisposableweiter Shoelaceund meine nette kleine Klasse wird zu diesem schrecklichen Durcheinander:

class Shoelace : IDisposable
{
    private AutoResetEvent waitHandle = new AutoResetEvent(false);
    bool tied = false;
    private bool disposed = false;

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~Shoelace()
    {
        Dispose(false);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!this.disposed)
        {
            if (disposing)
            {
                if (waitHandle != null)
                {
                    waitHandle.Close();
                    waitHandle = null;
                }
            }
            // No unmanaged resources to release otherwise they'd go here.
        }
        disposed = true;
    }
}

Oder (wie von Kommentatoren hervorgehoben), da es Shoelaceselbst keine nicht verwalteten Ressourcen gibt, könnte ich die einfachere Entsorgungsimplementierung verwenden, ohne den Dispose(bool)und Destructor zu benötigen :

class Shoelace : IDisposable
{
    private AutoResetEvent waitHandle = new AutoResetEvent(false);
    bool tied = false;

    public void Dispose()
    {
        if (waitHandle != null)
        {
            waitHandle.Close();
            waitHandle = null;
        }
        GC.SuppressFinalize(this);
    }
}

Beobachten Sie entsetzt, wie sich IDisposable ausbreitet

Richtig, das ist behoben. Aber jetzt wird sich FxCop beschweren, dass ein Shoeschafft Shoelace, so Shoemuss es IDisposableauch sein.

Und Driverschafft muss Shoeso Driversein IDisposable. Und Busschafft Driverso Busmuss sein IDisposableund so weiter.

Plötzlich verursacht meine kleine Änderung an Shoelaceviel Arbeit und mein Chef fragt sich, warum ich zur Kasse gehen muss Bus, um eine Änderung vorzunehmen Shoelace.

Die Frage

Wie verhindern Sie diese Ausbreitung IDisposableund stellen dennoch sicher, dass Ihre nicht verwalteten Objekte ordnungsgemäß entsorgt werden?

GrahamS
quelle
9
Eine außergewöhnlich gute Frage, ich glaube, die Antwort ist, ihre Verwendung zu minimieren und zu versuchen, die IDisposables auf hoher Ebene kurzlebig zu halten, aber dies ist nicht immer möglich (insbesondere, wenn diese IDisposables auf Interop mit C ++ - DLLs oder ähnlichem zurückzuführen sind). Schauen Sie sich die Antworten an.
Annakata
Okay, Dan, ich habe die Frage aktualisiert, um beide Methoden zur Implementierung von IDisposable auf Shoelace zu zeigen.
GrahamS
Ich bin normalerweise vorsichtig, wenn ich mich auf die Implementierungsdetails anderer Klassen verlasse, um mich zu schützen. Es macht keinen Sinn, es zu riskieren, wenn ich es leicht verhindern kann. Vielleicht bin ich übervorsichtig oder ich habe einfach zu lange als C-Programmierer verbracht, aber ich würde lieber den irischen Ansatz
wählen
@Dan: Die Nullprüfung wird weiterhin benötigt, um sicherzustellen, dass das Objekt selbst nicht auf null gesetzt wurde. In diesem Fall löst der Aufruf von waitHandle.Dispose () eine NullReferenceException aus.
Scott Dorman
1
Egal was passiert, Sie sollten tatsächlich immer noch die Dispose (bool) -Methode verwenden, wie in Ihrem Abschnitt "Implement IDisposable on Shoelace" gezeigt, da dies (abzüglich des Finalizers) das vollständige Muster ist. Nur weil eine Klasse IDisposable implementiert, heißt das nicht, dass sie einen Finalizer benötigt.
Scott Dorman

Antworten:

36

Sie können die Ausbreitung von IDisposable nicht wirklich "verhindern". Einige Klassen müssen entsorgt werden AutoResetEvent, und der effizienteste Weg besteht darin, dies in der Dispose()Methode zu tun , um den Overhead von Finalisierern zu vermeiden. Aber diese Methode muss irgendwie aufgerufen werden, so genau wie in Ihrem Beispiel die Klassen, die IDisposable kapseln oder enthalten, diese entsorgen müssen, also müssen sie auch verfügbar sein usw. Der einzige Weg, dies zu vermeiden, ist:

  • Vermeiden Sie nach Möglichkeit die Verwendung von IDisposable-Klassen, sperren oder warten Sie auf Ereignisse an einzelnen Orten, halten Sie teure Ressourcen an einem Ort usw.
  • Erstellen Sie sie nur, wenn Sie sie benötigen, und entsorgen Sie sie unmittelbar danach (das usingMuster).

In einigen Fällen kann IDisposable ignoriert werden, da es einen optionalen Fall unterstützt. Beispielsweise implementiert WaitHandle IDisposable, um einen benannten Mutex zu unterstützen. Wenn kein Name verwendet wird, führt die Dispose-Methode nichts aus. MemoryStream ist ein weiteres Beispiel, es verwendet keine Systemressourcen und seine Dispose-Implementierung führt auch nichts aus. Sorgfältiges Überlegen, ob eine nicht verwaltete Ressource verwendet wird oder nicht, kann lehrreich sein. So können Sie die verfügbaren Quellen für die .net-Bibliotheken untersuchen oder einen Dekompiler verwenden.

Grzenio
quelle
4
Der Rat, den Sie ignorieren können, weil die heutige Implementierung nichts bewirkt, ist schlecht, da eine zukünftige Version möglicherweise tatsächlich etwas Wichtiges in Dispose bewirkt, und jetzt ist es schwierig, ein Leck aufzuspüren.
Andy
1
"teure Ressourcen behalten", IDisposables sind nicht unbedingt teuer, in der Tat handelt es sich bei diesem Beispiel, das einen gleichmäßigen Wartegriff verwendet, um ein "geringes Gewicht".
Markmnl
20

In Bezug auf die Korrektheit können Sie die Verbreitung von IDisposable durch eine Objektbeziehung nicht verhindern, wenn ein übergeordnetes Objekt ein untergeordnetes Objekt erstellt und im Wesentlichen besitzt, das jetzt verfügbar sein muss. FxCop ist in dieser Situation korrekt und das übergeordnete Element muss IDisposable sein.

Sie können vermeiden, einer Blattklasse in Ihrer Objekthierarchie ein IDisposable hinzuzufügen. Dies ist nicht immer eine leichte Aufgabe, aber eine interessante Übung. Aus logischer Sicht gibt es keinen Grund, warum eine Schuhspitze wegwerfbar sein muss. Anstatt hier ein WaitHandle hinzuzufügen, ist es auch möglich, an der Stelle, an der es verwendet wird, eine Zuordnung zwischen einem ShoeLace und einem WaitHandle hinzuzufügen. Der einfachste Weg führt über eine Dictionary-Instanz.

Wenn Sie den WaitHandle an dem Punkt, an dem der WaitHandle tatsächlich verwendet wird, über eine Karte in eine lose Zuordnung verschieben können, können Sie diese Kette unterbrechen.

JaredPar
quelle
2
Es fühlt sich sehr seltsam an, dort eine Vereinigung zu bilden. Dieses AutoResetEvent ist für die Shoelace-Implementierung privat, daher wäre es meiner Meinung nach falsch, es öffentlich zugänglich zu machen.
GrahamS
@ AbrahamS, ich sage nicht, es öffentlich zu machen. Ich sage, kann es bis zu dem Punkt bewegt werden, an dem die Schnürsenkel gebunden sind. Wenn das Binden von Schnürsenkeln eine so komplizierte Aufgabe ist, sollte es sich möglicherweise um eine Schnürsenkelklasse handeln.
JaredPar
1
Könnten Sie Ihre Antwort mit einigen Codefragmenten von JaredPar erweitern? Ich kann mein Beispiel ein wenig dehnen, aber ich stelle mir vor, dass Shoelace einen Tyer-Thread erstellt und startet, der geduldig bei waitHandle.WaitOne () wartet. Der Shoelace würde waitHandle.Set () aufrufen, wenn der Thread mit dem Binden beginnen soll.
GrahamS
1
+1 dazu. Ich denke, es wäre seltsam, wenn nur Schnürsenkel gleichzeitig genannt würden, aber nicht die anderen. Überlegen Sie, was die aggregierte Wurzel sein soll. In diesem Fall sollte es IMHO Bus sein, obwohl ich mit der Domain nicht vertraut bin. In diesem Fall sollte der Bus den Waithandle und alle Operationen gegen den Bus enthalten, und alle untergeordneten Busse werden synchronisiert.
Johannes Gustafsson
16

Um eine IDisposableAusbreitung zu verhindern , sollten Sie versuchen, die Verwendung eines Einwegobjekts in einer einzigen Methode zusammenzufassen. Versuchen Sie Shoelaceanders zu gestalten :

class Shoelace { 
  bool tied = false; 

  public void Tie() {

    using (var waitHandle = new AutoResetEvent(false)) {

      // you can even pass the disposable to other methods
      OtherMethod(waitHandle);

      // or hold it in a field (but FxCop will complain that your class is not disposable),
      // as long as you take control of its lifecycle
      _waitHandle = waitHandle;
      OtherMethodThatUsesTheWaitHandleFromTheField();

    } 

  }
} 

Der Umfang des Wartehandles ist auf die TieMethode beschränkt, und die Klasse muss kein verfügbares Feld haben und muss daher nicht selbst verfügbar sein.

Da das Wait-Handle ein Implementierungsdetail innerhalb von ist Shoelace, sollte es seine öffentliche Schnittstelle in keiner Weise ändern, wie das Hinzufügen einer neuen Schnittstelle in seiner Deklaration. Was passiert dann, wenn Sie kein Einwegfeld mehr benötigen IDisposable? Entfernen Sie die Deklaration? Wenn Sie über die Shoelace Abstraktion nachdenken , erkennen Sie schnell, dass sie nicht durch Infrastrukturabhängigkeiten wie verschmutzt werden sollte IDisposable. IDisposablesollte für Klassen reserviert werden, deren Abstraktion eine Ressource enthält, die eine deterministische Bereinigung erfordert; dh für Klassen, in denen die Verfügbarkeit Teil der Abstraktion ist .

Jordão
quelle
1
Ich stimme voll und ganz zu, dass das Implementierungsdetail von Shoelace die öffentliche Schnittstelle nicht verschmutzen sollte, aber mein Punkt ist, dass es schwierig sein kann, dies zu vermeiden. Was Sie hier vorschlagen, wäre nicht immer möglich: Der Zweck von AutoResetEvent () ist die Kommunikation zwischen Threads, sodass der Umfang normalerweise über eine einzelne Methode hinausgeht.
GrahamS
@ AbrahamS: Deshalb habe ich gesagt, ich soll versuchen, es so zu gestalten. Sie können das Einwegprodukt auch an andere Methoden übergeben. Es bricht nur zusammen, wenn externe Anrufe an die Klasse den Lebenszyklus des Einwegartikels steuern. Ich werde die Antwort entsprechend aktualisieren.
Jordão
2
Entschuldigung, ich weiß, dass Sie das Einwegprodukt weitergeben können, aber ich sehe das immer noch nicht. In meinem Beispiel wird das AutoResetEventverwendet, um zwischen verschiedenen Threads zu kommunizieren, die innerhalb derselben Klasse arbeiten. Daher muss es eine Mitgliedsvariable sein. Sie können den Umfang nicht auf eine Methode beschränken. (Stellen Sie sich z. B. vor, ein Thread wartet nur auf etwas Arbeit, indem er blockiert waitHandle.WaitOne(). Der Haupt-Thread ruft dann die shoelace.Tie()Methode auf, die nur a ausführt waitHandle.Set()und sofort zurückkehrt.)
GrahamS
3

Dies ist im Grunde das, was passiert, wenn Sie Komposition oder Aggregation mit Einwegklassen mischen. Wie bereits erwähnt, besteht der erste Ausweg darin, den waitHandle aus dem Schnürsenkel herauszuarbeiten.

Allerdings können Sie das Einwegmuster erheblich reduzieren, wenn Sie nicht über nicht verwaltete Ressourcen verfügen. (Ich suche noch nach einer offiziellen Referenz dafür.)

Sie können jedoch den Destruktor und GC.SuppressFinalize (this) weglassen. und vielleicht die virtuelle Leere aufräumen (bool disposing) ein bisschen.

Henk Holterman
quelle
Danke Henk. Wenn ich die Schaffung von Waithandle aus Shoelace nahm dann jemand hat immer noch zu entsorgen es irgendwo so wie würde das jemand wissen , dass Schnürsenkel mit ihm fertig ist (und das Shoelace es nicht auf andere Klassen übergeben)?
GrahamS
Sie müssen nicht nur die Erschaffung, sondern auch die "Verantwortung" für den Waithandle verschieben. Die Antwort von jaredPar hat einen interessanten Anfang einer Idee.
Henk Holterman
Dieser Blog behandelt die Implementierung von IDisposable und befasst sich insbesondere mit der Vereinfachung der Aufgabe, indem vermieden wird, dass verwaltete und nicht verwaltete Ressourcen in einem Blog
PaulS
3

Interessanterweise, wenn Driverwie oben definiert ist:

class Driver
{
    Shoe[] shoes = { new Shoe(), new Shoe() };
}

Wenn Shoedann gemacht wird IDisposable, beschwert sich FxCop (v1.36) nicht, dass dies Driverauch sein sollte IDisposable.

Wenn es jedoch so definiert ist:

class Driver
{
    Shoe leftShoe = new Shoe();
    Shoe rightShoe = new Shoe();
}

dann wird es sich beschweren.

Ich vermute, dass dies nur eine Einschränkung von FxCop ist und keine Lösung, da in der ersten Version die ShoeInstanzen noch von der erstellt werden Driverund noch irgendwie entsorgt werden müssen.

GrahamS
quelle
2
Wenn Show IDisposable implementiert, ist das Erstellen in einem Feldinitialisierer gefährlich. Interessant, dass FXCop solche Initialisierungen nicht zulässt, da sie in C # nur mit ThreadStatic-Variablen (geradezu abscheulich) sicher durchgeführt werden können. In vb.net ist es möglich, IDisposable-Objekte ohne ThreadStatic-Variablen sicher zu initialisieren. Der Code ist immer noch hässlich, aber nicht ganz so abscheulich. Ich wünschte, MS würde eine gute Möglichkeit bieten, solche Feldinitialisierer sicher zu verwenden.
Supercat
1
@ Supercat: Danke. Können Sie erklären, warum es gefährlich ist? Ich vermute, wenn eine Ausnahme in einem der ShoeKonstruktoren ausgelöst shoeswürde, würde dies in einem schwierigen Zustand bleiben?
GrahamS
3
Wenn man nicht weiß, dass ein bestimmter Typ von IDisposable sicher aufgegeben werden kann, sollte man sicherstellen, dass jedes erstellte Objekt entsorgt wird. Wenn ein IDisposable im Feldinitialisierer eines Objekts erstellt wird, aber eine Ausnahme ausgelöst wird, bevor das Objekt vollständig erstellt wurde, werden das Objekt und alle seine Felder verlassen. Selbst wenn die Konstruktion des Objekts in eine Factory-Methode eingeschlossen ist, die einen "try" -Block verwendet, um zu erkennen, dass die Konstruktion fehlgeschlagen ist, ist es für die Factory schwierig, Verweise auf die Objekte zu erhalten, die entsorgt werden müssen.
Supercat
3
Die einzige Möglichkeit, mit der ich in C # umgehen kann, besteht darin, eine ThreadStatic-Variable zu verwenden, um eine Liste der Objekte zu führen, die entsorgt werden müssen, wenn der aktuelle Konstruktor auslöst. Lassen Sie dann jeden Feldinitialisierer jedes Objekt registrieren, während es erstellt wird, lassen Sie die Liste vom Werk löschen, wenn es erfolgreich abgeschlossen wurde, und verwenden Sie eine "finally" -Klausel, um alle Elemente aus der Liste zu entsorgen, wenn es nicht gelöscht wurde. Das wäre ein praktikabler Ansatz, aber ThreadStatic-Variablen sind hässlich. In vb.net könnte man eine ähnliche Technik verwenden, ohne ThreadStatic-Variablen zu benötigen.
Supercat
3

Ich glaube nicht, dass es eine technische Möglichkeit gibt, die Verbreitung von IDisposable zu verhindern, wenn Sie Ihr Design so eng miteinander verbinden. Man sollte sich dann fragen, ob das Design stimmt.

In Ihrem Beispiel halte ich es für sinnvoll, dass der Schuh den Schnürsenkel besitzt, und vielleicht sollte der Fahrer seine Schuhe besitzen. Der Bus sollte jedoch nicht den Fahrer besitzen. Normalerweise folgen Busfahrer Bussen nicht zum Schrottplatz :) Bei Fahrern und Schuhen stellen Fahrer selten ihre eigenen Schuhe her, was bedeutet, dass sie diese nicht wirklich "besitzen".

Ein alternatives Design könnte sein:

class Bus
{
   IDriver busDriver = null;
   public void SetDriver(IDriver d) { busDriver = d; }
}

class Driver : IDriver
{
   IShoePair shoes = null;
   public void PutShoesOn(IShoePair p) { shoes = p; }
}

class ShoePairWithDisposableLaces : IShoePair, IDisposable
{
   Shoelace lace = new Shoelace();
}

class Shoelace : IDisposable
{
   ...
}

Das neue Design ist leider komplizierter, da zusätzliche Klassen erforderlich sind, um konkrete Instanzen von Schuhen und Fahrern zu instanziieren und zu entsorgen. Diese Komplikation ist jedoch mit dem zu lösenden Problem verbunden. Das Gute ist, dass Busse nicht mehr nur zum Entsorgen von Schnürsenkeln wegwerfbar sein müssen.

Joh
quelle
2
Dies löst das ursprüngliche Problem jedoch nicht wirklich. Es könnte FxCop ruhig halten, aber etwas sollte den Schnürsenkel noch entsorgen.
AndrewS
Ich denke, alles, was Sie getan haben, ist, das Problem woanders hin zu verschieben. ShoePairWithDisposableLaces besitzt jetzt Shoelace (), daher muss es auch IDisposable gemacht werden - also wer entsorgt die Schuhe? Überlassen Sie dies einem IoC-Container?
GrahamS
@ Andrews: Zwar muss noch etwas über Fahrer, Schuhe und Schnürsenkel verfügen, aber die Entsorgung sollte nicht durch Busse eingeleitet werden. @ AbrahamS: Sie haben Recht, ich verschiebe das Problem woanders hin, weil ich glaube, dass es woanders hingehört. Typischerweise gibt es eine Klasse, die Schuhe instanziiert, die auch für die Entsorgung von Schuhen verantwortlich ist. Ich habe den Code so bearbeitet, dass ShoePairWithDisposableLaces auch verfügbar ist.
Joh
@ Joh: Danke. Wie Sie sagen, besteht das Problem beim Verschieben an einen anderen Ort darin, dass Sie viel mehr Komplexität schaffen. Wenn ShoePairWithDisposableLaces von einer anderen Klasse erstellt wird, muss diese Klasse dann nicht benachrichtigt werden, wenn der Fahrer mit seinen Schuhen fertig ist, damit er sie ordnungsgemäß entsorgen kann ()?
GrahamS
1
@ Abraham: Ja, ein Benachrichtigungsmechanismus wäre erforderlich. Beispielsweise könnte eine Entsorgungsrichtlinie verwendet werden, die auf Referenzzählungen basiert. Es gibt einige zusätzliche Komplexität, aber wie Sie wahrscheinlich wissen, "Es gibt keinen freien Schuh" :)
Joh
1

Wie wäre es mit Inversion of Control?

class Bus
{
    private Driver busDriver;

    public Bus(Driver busDriver)
    {
        this.busDriver = busDriver;
    }
}

class Driver
{
    private Shoe[] shoes;

    public Driver(Shoe[] shoes)
    {
        this.shoes = shoes;
    }
}

class Shoe
{
    private Shoelace lace;

    public Shoe(Shoelace lace)
    {
        this.lace = lace;
    }
}

class Shoelace
{
    bool tied;
    private AutoResetEvent waitHandle;

    public Shoelace(bool tied, AutoResetEvent waitHandle)
    {
        this.tied = tied;
        this.waitHandle = waitHandle;
    }
}

class Program
{
    static void Main(string[] args)
    {
        using (var leftShoeWaitHandle = new AutoResetEvent(false))
        using (var rightShoeWaitHandle = new AutoResetEvent(false))
        {
            var bus = new Bus(new Driver(new[] {new Shoe(new Shoelace(false, leftShoeWaitHandle)),new Shoe(new Shoelace(false, rightShoeWaitHandle))}));
        }
    }
}
Darragh
quelle
Jetzt haben Sie es zum Anruferproblem gemacht, und Shoelace kann sich nicht darauf verlassen, dass das Wartehandle verfügbar ist, wenn es benötigt wird.
Andy
@andy Was meinst du mit "Schnürsenkel kann nicht davon abhängen, dass das Wartehandle verfügbar ist, wenn es benötigt wird"? Es wird an den Konstruktor übergeben.
Darragh
Etwas anderes könnte den Zustand der automatischen Ereignisse vor der Übergabe an Shoelace durcheinander gebracht haben, und es könnte damit beginnen, dass sich das ARE in einem schlechten Zustand befindet. Etwas anderes könnte den Zustand des ARE beeinträchtigen, während Shoelace sein Ding macht, was dazu führt, dass Shoelace das Falsche macht. Aus demselben Grund sperren Sie nur private Mitglieder. "Vermeiden Sie im Allgemeinen das Sperren von Instanzen, die außerhalb Ihrer Code-Kontrolle liegen" docs.microsoft.com/en-us/dotnet/csharp/language-reference/…
Andy
Ich bin damit einverstanden, nur private Mitglieder zu sperren. Aber es scheint, als wären die Wartegriffe anders. Tatsächlich scheint es sinnvoller zu sein, sie an das Objekt zu übergeben, da dieselbe Instanz für die Kommunikation zwischen Threads verwendet werden muss. Dennoch denke ich, dass IoC eine nützliche Lösung für das OP-Problem bietet.
Darragh
Angesichts der Tatsache, dass das OP-Beispiel den Waithandle als privates Mitglied hatte, ist die sichere Annahme, dass das Objekt exklusiven Zugriff darauf erwartet. Wenn Sie das Handle außerhalb der Instanz sichtbar machen, wird dies verletzt. Normalerweise kann ein IoC für solche Dinge gut sein, aber nicht, wenn es um Threading geht.
Andy