Jak zapobiegasz rozprzestrzenianiu się IDisposable na wszystkie Twoje klasy?

138

Zacznij od tych prostych zajęć ...

Powiedzmy, że mam prosty zestaw klas, taki jak ten:

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 Busma Driver, Driverma dwa Shoes, każdy Shoema Shoelace. Bardzo głupie.

Dodaj obiekt IDisposable do Shoelace

Później zdecydowałem, że niektóre operacje na serwerze Shoelacemogą być wielowątkowe, więc dodaję EventWaitHandleznak, aby wątki się komunikowały. Więc Shoelaceteraz wygląda to tak:

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

Zaimplementuj IDisposable na Shoelace

Ale teraz FxCop Microsoftu będzie narzekał: „Zaimplementuj IDisposable na 'Shoelace', ponieważ tworzy elementy członkowskie następujących typów IDisposable: 'EventWaitHandle'."

Dobra, wdrażam IDisposabledalej Shoelacei moja zgrabna mała klasa staje się strasznym bałaganem:

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;
    }
}

Lub (jak podkreślają komentatorzy), ponieważ Shoelacesam nie ma niezarządzanych zasobów, mogę użyć prostszej implementacji usuwania bez potrzeby Dispose(bool)i Destructor:

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);
    }
}

Oglądaj z przerażeniem, jak rozprzestrzenia się IDisposable

Tak, to naprawione. Ale teraz FxCop będzie narzekać, że Shoetworzy plik Shoelace, więc też Shoemusi być IDisposable.

I Drivertworzy Shoetak Drivermusi być IDisposable. I Bustworzy Drivertak Busmusi być IDisposablei tak dalej.

Nagle moja mała zmiana w aplikacji Shoelacepowoduje dużo pracy, a mój szef zastanawia się, dlaczego muszę się wymeldować, Busaby dokonać zmiany Shoelace.

Pytanie

Jak zapobiec temu rozprzestrzenianiu się IDisposable, ale jednocześnie zapewnić, że niezarządzane obiekty są prawidłowo usuwane?

GrahamS
źródło
9
Wyjątkowo dobre pytanie, uważam, że odpowiedzią jest zminimalizowanie ich użycia i staranie się, aby narzędzia IDisposables wysokiego poziomu były krótkotrwałe z użyciem, ale nie zawsze jest to możliwe (szczególnie tam, gdzie te IDisposables są ze względu na współdziałanie z bibliotekami DLL C ++ lub podobnymi). Poszukaj odpowiedzi.
annakata
Dobra Dan, zaktualizowałem pytanie, aby pokazać obie metody implementacji IDisposable w Shoelace.
GrahamS,
Zwykle nie mogę polegać na szczegółach implementacji innych klas, aby mnie chronić. Nie ma sensu ryzykować, jeśli mogę łatwo temu zapobiec. Może jestem zbyt ostrożny, a może po prostu spędziłem zbyt dużo czasu jako programista C, ale wolę podejście irlandzkie: „To be sure, to be sure” :)
GrahamS
@Dan: sprawdzenie wartości null jest nadal potrzebne, aby upewnić się, że sam obiekt nie został ustawiony na wartość null, w takim przypadku wywołanie waitHandle.Dispose () zgłosi wyjątek NullReferenceException.
Scott Dorman
1
Bez względu na wszystko, w rzeczywistości nadal powinieneś używać metody Dispose (bool), jak pokazano w sekcji „Implement IDisposable on Shoelace”, ponieważ to (bez finalizatora) jest pełnym wzorcem. To, że klasa zawiera IDisposable, nie oznacza, że ​​potrzebuje finalizatora.
Scott Dorman

Odpowiedzi:

36

Tak naprawdę nie można „zapobiec” rozprzestrzenianiu się IDisposable. Niektóre klasy muszą zostać usunięte, AutoResetEventa najbardziej efektywnym sposobem jest zrobienie tego w Dispose()metodzie, aby uniknąć narzutu finalizatorów. Ale ta metoda musi być jakoś wywołana, więc dokładnie tak, jak w twoim przykładzie klasy, które zawierają lub zawierają IDisposable, muszą je usunąć, więc muszą być również jednorazowe itp. Jedynym sposobem uniknięcia tego jest:

  • unikaj używania klas IDisposable tam, gdzie to możliwe, blokuj lub czekaj na zdarzenia w pojedynczych miejscach, trzymaj drogie zasoby w jednym miejscu itp
  • twórz je tylko wtedy, gdy ich potrzebujesz i wyrzucaj je zaraz po ( usingwzorze)

W niektórych przypadkach IDisposable można zignorować, ponieważ obsługuje opcjonalną wielkość liter. Na przykład WaitHandle implementuje IDisposable do obsługi nazwanego Mutex. Jeśli nazwa nie jest używana, metoda Dispose nic nie robi. MemoryStream to kolejny przykład, który nie używa żadnych zasobów systemowych, a jego implementacja Dispose również nic nie robi. Dokładne przemyślenie, czy niezarządzany zasób jest używany, czy nie, może być pouczające. Podobnie można sprawdzić dostępne źródła bibliotek .net lub użyć dekompilatora.

Grzenio
źródło
4
Rada, że ​​możesz to zignorować, ponieważ dzisiejsza implementacja nic nie robi, jest zła, ponieważ przyszła wersja może faktycznie zrobić coś ważnego w Dispose, a teraz masz trudny do wyśledzenia wyciek.
Andy
1
„zachowaj drogie zasoby”, IDisposable niekoniecznie są drogie, w istocie ten przykład, który używa uchwytu równego czekania, jest o „niewielkiej wadze”.
markmnl
20

Jeśli chodzi o poprawność, nie można zapobiec rozprzestrzenianiu się IDisposable poprzez relację między obiektami, jeśli obiekt nadrzędny tworzy i zasadniczo jest właścicielem obiektu podrzędnego, który teraz musi być jednorazowy. FxCop jest poprawny w tej sytuacji, a rodzic musi być IDisposable.

Możesz uniknąć dodawania IDisposable do klasy liścia w hierarchii obiektów. Nie zawsze jest to łatwe zadanie, ale jest to ciekawe ćwiczenie. Z logicznego punktu widzenia nie ma powodu, dla którego sznurowadło musi być jednorazowe. Zamiast dodawać tutaj WaitHandle, można również dodać skojarzenie między ShoeLace i WaitHandle w miejscu, w którym jest używany. Najprostszym sposobem jest użycie instancji Dictionary.

Jeśli możesz przenieść WaitHandle do luźnego powiązania za pośrednictwem mapy w miejscu, w którym WaitHandle jest faktycznie używany, możesz przerwać ten łańcuch.

JaredPar
źródło
2
Tworzenie tam skojarzeń jest bardzo dziwne. To AutoResetEvent jest prywatne dla implementacji Shoelace, więc ujawnienie go publicznie byłoby moim zdaniem niewłaściwe.
GrahamS
@GrahamS, nie mówię, że ujawniaj to publicznie. Mówię, czy można to przenieść do punktu, w którym sznurówki są zawiązane. Być może, jeśli wiązanie sznurówek do butów jest tak skomplikowanym zadaniem, powinno to być klasą sznurowadeł.
JaredPar
1
Czy możesz rozszerzyć swoją odpowiedź o fragmenty kodu JaredPar? Może trochę rozciągam mój przykład, ale wyobrażam sobie, że Shoelace tworzy i uruchamia nić Tyer, która cierpliwie czeka na waitHandle.WaitOne (). Sznurowadło wywoływało waitHandle.Set (), gdy chciał, aby nić zaczęła się wiązać.
GrahamS,
1
+1 w tej sprawie. Myślę, że byłoby dziwne, gdyby tylko Shoelace byłby nazywany jednocześnie, a pozostałe nie. Pomyśl, co powinno być głównym korzeniem. W tym przypadku powinno to być Bus, IMHO, chociaż nie znam tej domeny. W takim przypadku magistrala powinna zawierać waithandle, a wszystkie operacje na magistrali i wszystkie jego elementy podrzędne zostaną zsynchronizowane.
Johannes Gustafsson,
16

Aby zapobiec IDisposablerozprzestrzenianiu się, powinieneś spróbować hermetyzować użycie jednorazowego obiektu wewnątrz jednej metody. Spróbuj zaprojektować Shoelaceinaczej:

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();

    } 

  }
} 

Zakres uchwytu oczekiwania jest ograniczony do Tie metody, a klasa nie musi mieć pola jednorazowego, a więc sama nie musi być jednorazowa.

Ponieważ uchwyt oczekiwania jest szczegółem implementacji wewnątrz elementu Shoelace, nie powinien on w żaden sposób zmieniać swojego publicznego interfejsu, na przykład poprzez dodanie nowego interfejsu w swojej deklaracji. Co się wtedy stanie, gdy nie będziesz już potrzebować pola jednorazowego, czy usuniesz IDisposabledeklarację? Jeśli pomyślisz o Shoelace abstrakcji , szybko zdasz sobie sprawę, że nie powinna być zanieczyszczona zależnościami infrastruktury, takimi jak IDisposable. IDisposablepowinny być zarezerwowane dla klas, których abstrakcja obejmuje zasób wymagający deterministycznego czyszczenia; tj. dla klas, w których rozporządzalność jest częścią abstrakcji .

Jordão
źródło
1
Całkowicie się zgadzam, że szczegóły implementacji Shoelace nie powinny zanieczyszczać interfejsu publicznego, ale chodzi mi o to, że uniknięcie tego może być trudne. To, co tutaj sugerujesz, nie zawsze byłoby możliwe: celem AutoResetEvent () jest komunikacja między wątkami, więc jej zakres zwykle wykracza poza jedną metodę.
GrahamS
@GrahamS: dlatego powiedziałem, żeby spróbować zaprojektować to w ten sposób. Możesz również przekazać jednorazowe inne metody. Zepsuje się tylko wtedy, gdy zewnętrzne wywołania klasy kontrolują cykl życia jednorazowego użytku. Odpowiednio zaktualizuję odpowiedź.
Jordão,
2
przepraszam, wiem, że możesz przekazać jednorazowe urządzenie, ale nadal nie widzę, aby to działało. W moim przykładzie AutoResetEventjest używany do komunikacji między różnymi wątkami, które działają w tej samej klasie, więc musi być zmienną składową. Nie możesz ograniczyć jego zakresu do metody. (np. wyobraź sobie, że jeden wątek po prostu czeka na pracę, blokując się waitHandle.WaitOne(). Następnie główny wątek wywołuje shoelace.Tie()metodę, która po prostu wykonuje a waitHandle.Set()i natychmiast zwraca).
GrahamS,
3

Zasadniczo dzieje się tak, gdy mieszasz Kompozycję lub Agregację z klasami jednorazowymi. Jak wspomniano, pierwszym wyjściem byłoby zrefaktoryzowanie waitHandle z sznurówki.

Powiedziawszy to, możesz znacznie zmniejszyć wzorzec Disposable, gdy nie masz niezarządzanych zasobów. (Nadal szukam oficjalnego odniesienia do tego.)

Ale możesz pominąć destruktor i GC.SuppressFinalize (this); i może trochę wyczyścić wirtualną pustkę Dispose (bool dispose).

Henk Holterman
źródło
Dzięki Henk. Gdybym wziął tworzenie waitHandle z Shoelace, to ktoś nadal musi go gdzieś pozbyć, więc skąd ten ktoś może wiedzieć, że Shoelace jest skończone z nim (i że Shoelace nie przekazał go na żadne inne zajęcia)?
GrahamS
Będziesz musiał przenieść nie tylko tworzenie, ale także „odpowiedzialność za” waithandle. Odpowiedź jaredPar ma ciekawy początek pomysłu.
Henk Holterman
Ten blog dotyczy implementacji IDisposable, aw szczególności dotyczy sposobów uproszczenia zadania poprzez unikanie mieszania zarządzanych i niezarządzanych zasobów w jednej klasie blog.stephencleary.com/2009/08/…
PaulS
3

Co ciekawe, jeśli Driverzdefiniowano jak powyżej:

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

Wtedy, kiedy Shoejest zrobiony IDisposable, FxCop (v1.36) nie narzeka, że ​​tak Driverpowinno byćIDisposable .

Jednak jeśli jest zdefiniowane w ten sposób:

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

wtedy będzie narzekać.

Podejrzewam, że jest to tylko ograniczenie FxCop, a nie rozwiązanie, ponieważ w pierwszej wersji Shoeinstancje są nadal tworzone przez Driveri nadal trzeba je jakoś pozbyć.

GrahamS
źródło
2
Jeśli Show implementuje IDisposable, utworzenie go w inicjatorze pola jest niebezpieczne. Ciekawe, że FXCop nie pozwoliłby na takie inicjalizacje, ponieważ w C # jedynym sposobem na ich bezpieczne wykonanie są zmienne ThreadStatic (wręcz ohydne). W vb.net można bezpiecznie inicjować obiekty IDisposable bez zmiennych ThreadStatic. Kod jest nadal brzydki, ale nie tak ohydny. Chciałbym, żeby firma MS zapewniła miły sposób na bezpieczne używanie takich inicjatorów.
supercat
1
@supercat: dzięki. Czy możesz wyjaśnić, dlaczego jest to niebezpieczne? Domyślam się, że gdyby wyjątek został wrzucony w jednym z Shoekonstruktorów, to shoeszostałby w trudnym stanie?
GrahamS
3
O ile nie wiadomo, że określony typ IDisposable można bezpiecznie porzucić, należy upewnić się, że każdy utworzony obiekt zostanie Dispose'd. Jeśli IDisposable zostanie utworzony w inicjatorze pola obiektu, ale wyjątek zostanie zgłoszony przed całkowitym skonstruowaniem obiektu, obiekt i wszystkie jego pola zostaną porzucone. Nawet jeśli konstrukcja obiektu jest opakowana metodą fabryczną, która używa bloku „try” do wykrycia niepowodzenia konstrukcji, fabryka ma trudności z uzyskaniem odniesień do obiektów wymagających usunięcia.
supercat
3
Jedynym sposobem, w jaki wiem, jak sobie z tym poradzić w C #, byłoby użycie zmiennej ThreadStatic do przechowywania listy obiektów, które będą wymagały usunięcia, jeśli aktualny konstruktor zgłosi. Następnie niech każdy inicjator pola zarejestruje każdy obiekt podczas jego tworzenia, niech fabryka wyczyści listę, jeśli zakończy się pomyślnie, i użyj klauzuli „last”, aby usunąć wszystkie elementy z listy, jeśli nie została wyczyszczona. To byłoby wykonalne podejście, ale zmienne ThreadStatic są brzydkie. W vb.net można by użyć podobnej techniki bez konieczności stosowania zmiennych ThreadStatic.
supercat
3

Nie sądzę, aby istniał techniczny sposób zapobiegania rozprzestrzenianiu się IDisposable, jeśli projekt będzie tak ściśle powiązany. Należy się więc zastanowić, czy projekt jest właściwy.

W twoim przykładzie myślę, że sensowne jest, aby but miał sznurowadło, a może kierowca powinien posiadać swoje buty. Jednak autobus nie powinien być właścicielem kierowcy. Zazwyczaj kierowcy autobusów nie podążają za autobusami na złomowisko :) W przypadku kierowców i butów kierowcy rzadko robią własne buty, czyli tak naprawdę ich nie „posiadają”.

Alternatywnym projektem może być:

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
{
   ...
}

Nowy projekt jest niestety bardziej skomplikowany, ponieważ wymaga dodatkowych klas w celu utworzenia instancji i usunięcia konkretnych instancji butów i sterowników, ale ta komplikacja jest nieodłącznie związana z rozwiązywaniem problemu. Dobrą rzeczą jest to, że autobusy nie muszą już być jednorazowego użytku tylko w celu pozbycia się sznurowadeł.

Joh
źródło
2
To jednak tak naprawdę nie rozwiązuje pierwotnego problemu. Może to sprawić, że FxCop będzie cicho, ale coś nadal powinno pozbyć się sznurowadła.
AndrewS,
Myślę, że wszystko, co zrobiłeś, to przeniesienie problemu gdzie indziej. ShoePairWithDisposableLaces posiada teraz Shoelace (), więc musi być również przystosowany do jednorazowego użytku - więc kto pozbywa się butów? Czy zostawiasz to do obsługi jakiegoś kontenera IoC?
GrahamS,
@AndrewS: Rzeczywiście, coś musi jeszcze pozbyć się kierowców, butów i sznurówek, ale nie powinny być inicjowane przez autobusy. @GrahamS: Masz rację, przenoszę problem gdzie indziej, ponieważ uważam, że należy on gdzie indziej. Zwykle istniałaby klasa tworząca instancję butów, która byłaby również odpowiedzialna za usuwanie butów. Zmodyfikowałem kod, aby ShoePairWithDisposableLaces również był jednorazowy.
Joh
@Joh: Dzięki. Jak mówisz, problem z przeniesieniem go w inne miejsce polega na tym, że tworzysz o wiele bardziej złożoność. Jeśli ShoePairWithDisposableLaces jest tworzona przez jakąś inną klasę, to czy ta klasa nie musiałaby być powiadamiana, gdy Kierowca skończył z butami, aby mógł prawidłowo zutylizować () z nich?
GrahamS,
1
@Graham: tak, potrzebny byłby jakiś mechanizm powiadamiania. Można na przykład zastosować politykę usuwania opartą na liczbie referencji. Jest trochę dodaje złożoność, ale jak zapewne wiesz, „Nie ma czegoś takiego jak darmowy buta” :)
Jn
1

A co powiesz na użycie 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
źródło
Teraz sprawiłeś, że problem dzwoniących, a Shoelace nie może polegać na tym, że uchwyt oczekiwania będzie dostępny, gdy tego potrzebuje.
Andy
@andy Co masz na myśli, mówiąc „Sznurowadło nie może polegać na tym, że uchwyt oczekiwania będzie dostępny, gdy będzie potrzebny”? Jest przekazywany konstruktorowi.
Darragh
Coś innego mogło zepsuć stan autoodzyskiwania przed podaniem go Shoelace i może zacząć się od ARE w złym stanie; coś innego może zaszkodzić stanowi ARE, podczas gdy Shoelace robi swoje, powodując, że Shoelace zrobi coś złego. Z tego samego powodu blokujesz tylko prywatnych członków. „Ogólnie… unikaj blokowania wystąpień poza kontrolą kodów” docs.microsoft.com/en-us/dotnet/csharp/language-reference/…
Andy
Zgadzam się z blokowaniem tylko prywatnych członków. Ale wygląda na to, że uchwyty czekania są inne. W rzeczywistości wydaje się bardziej przydatne, aby przekazać je do obiektu, ponieważ ta sama instancja musi być używana do komunikacji między wątkami. Niemniej uważam, że IoC stanowi przydatne rozwiązanie problemu PO.
Darragh
Biorąc pod uwagę, że przykład PO posiadał waithandle jako członek prywatny, bezpieczne założenie jest takie, że obiekt oczekuje wyłącznego dostępu do niego. Udostępnianie uchwytu widocznego poza instancją narusza to. Zwykle IoC może być dobre do takich rzeczy, ale nie w przypadku wątków.
Andy