Mettre en œuvre correctement IDisposable

145

Dans mes classes, j'implémente IDisposable comme suit:

public class User : IDisposable
{
    public int id { get; protected set; }
    public string name { get; protected set; }
    public string pass { get; protected set; }

    public User(int UserID)
    {
        id = UserID;
    }
    public User(string Username, string Password)
    {
        name = Username;
        pass = Password;
    }

    // Other functions go here...

    public void Dispose()
    {
        // Clear all property values that maybe have been set
        // when the class was instantiated
        id = 0;
        name = String.Empty;
        pass = String.Empty;
    }
}

Dans VS2012, mon analyse de code dit d'implémenter correctement IDisposable, mais je ne suis pas sûr de ce que j'ai fait de mal ici.
Le texte exact est le suivant:

CA1063 Implémenter correctement IDisposable Fournit une implémentation remplaçable de Dispose (bool) sur 'User' ou marque le type comme scellé. Un appel à Dispose (false) ne doit nettoyer que les ressources natives. Un appel à Dispose (true) doit nettoyer les ressources gérées et natives. stman User.cs 10

Pour référence: CA1063: implémenter correctement IDisposable

J'ai lu cette page, mais j'ai bien peur de ne pas vraiment comprendre ce qui doit être fait ici.

Si quelqu'un peut expliquer en termes plus simples quel est le problème et / ou comment IDisposable doit être implémenté, cela aidera vraiment!

Ortund
la source
1
Est-ce tout le code à l'intérieur Dispose?
Claudio Redi
42
Vous devez implémenter votre méthode Dispose () pour appeler la méthode Dispose () sur l'un des membres de votre classe. Aucun de ces membres n'en a un. Vous ne devez donc pas implémenter IDisposable. La réinitialisation des valeurs de propriété est inutile.
Hans Passant
13
Il vous suffit de mettre en œuvre IDispoablesi vous avez des ressources non gérés à disposer de (ce qui inclut les ressources non gérés qui sont enveloppées ( SqlConnection, FileStream, etc.). Vous ne et ne devrait pas mettre en œuvre IDisposablesi vous n'avez réussi ressources comme ici. Ceci est, l' OMI, un problème majeur avec l'analyse de code. Il est très bon pour vérifier les petites règles stupides, mais pas bon pour vérifier les erreurs conceptuelles.
jason
51
Cela me dérange assez que certaines personnes préfèrent voter contre et voir cette question close plutôt que d'essayer d'aider une personne qui a clairement mal compris un concept. C'est dommage.
Ortund
2
Donc, ne pas voter contre, ne pas voter pour, laissez le message à zéro et fermez la question avec un pointeur utile.
tjmoore

Réponses:

113

Ce serait l'implémentation correcte, même si je ne vois rien dont vous avez besoin pour disposer dans le code que vous avez publié. Vous ne devez mettre en œuvre que IDisposablelorsque:

  1. Vous avez des ressources non gérées
  2. Vous vous accrochez à des références d'objets qui sont eux-mêmes jetables.

Rien dans le code que vous avez publié ne doit être supprimé.

public class User : IDisposable
{
    public int id { get; protected set; }
    public string name { get; protected set; }
    public string pass { get; protected set; }

    public User(int userID)
    {
        id = userID;
    }
    public User(string Username, string Password)
    {
        name = Username;
        pass = Password;
    }

    // Other functions go here...

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

    protected virtual void Dispose(bool disposing)
    {
        if (disposing) 
        {
            // free managed resources
        }
        // free native resources if there are any.
    }
}
Daniel Mann
la source
2
On m'a dit quand j'ai commencé à écrire en C # qu'il était préférable d'utiliser using(){ }autant que possible, mais pour ce faire, vous devez implémenter IDisposable, donc en général, je préfère accéder à une classe via des utilisations, en particulier. si je n'ai besoin que du cours dans une ou deux fonctions
Ortund
62
@Ortund Vous avez mal compris. Il est préférable d'utiliser un usingbloc lorsque la classe implémente IDisposable . Si vous n'avez pas besoin qu'une classe soit jetable, ne l'implémentez pas. Cela ne sert à rien.
Daniel Mann
5
@DanielMann La sémantique d'un usingbloc a tendance à être attrayante au-delà de l' IDisposableinterface seule, cependant. J'imagine qu'il y a eu plus que quelques abus IDisposablejuste à des fins de cadrage.
Thomas
1
Tout comme une note d'accompagnement, si vous avez des ressources non gérées à libérer, vous devez inclure Finalizer appelant Dispose (false), cela permettra à GC d'appeler Finalizer lors de la récupération de place (au cas où Dispose n'a pas encore été appelé) et correctement libéré non géré Ressources.
mariozski
4
Sans un finaliseur dans votre implémentation, l'appel GC.SuppressFinalize(this);est inutile. Comme @mariozski l'a souligné, un finaliseur aiderait à s'assurer qu'il Disposeest appelé du tout si la classe n'est pas utilisée dans un usingbloc.
Haymo Kutschbach
57

Tout d'abord, vous n'avez pas besoin de "nettoyer" les strings et ints - ils seront automatiquement pris en charge par le ramasse-miettes. La seule chose qui doit être nettoyée Disposeest les ressources non gérées ou les ressources gérées qui implémentent IDisposable.

Cependant, en supposant qu'il ne s'agisse que d'un exercice d'apprentissage, la méthode recommandée pour l'implémentation IDisposableest d'ajouter un «cran de sécurité» pour s'assurer que les ressources ne sont pas éliminées deux fois:

public void Dispose()
{
    Dispose(true);

    // Use SupressFinalize in case a subclass 
    // of this type implements a finalizer.
    GC.SuppressFinalize(this);   
}
protected virtual void Dispose(bool disposing)
{
    if (!_disposed)
    {
        if (disposing) 
        {
            // Clear all property values that maybe have been set
            // when the class was instantiated
            id = 0;
            name = String.Empty;
            pass = String.Empty;
        }

        // Indicate that the instance has been disposed.
        _disposed = true;   
    }
}
D Stanley
la source
3
+1, avoir un indicateur pour s'assurer que le code de nettoyage n'est exécuté qu'une seule fois est bien, bien mieux que de définir les propriétés sur null ou autre (d'autant plus que cela interfère avec la readonlysémantique)
Thomas
+1 pour utiliser le code utilisateur (même s'il sera nettoyé automatiquement) pour clarifier ce qui s'y passe. Aussi, pour ne pas être un marin salé et l'avoir martelé pour avoir fait une petite erreur en apprenant comme beaucoup d'autres ici.
Murphybro2
42

L'exemple suivant montre les meilleures pratiques générales pour implémenter l' IDisposableinterface. Référence

Gardez à l'esprit que vous n'avez besoin d'un destructeur (finaliseur) que si vous avez des ressources non gérées dans votre classe. Et si vous ajoutez un destructeur, vous devez supprimer la finalisation dans le Dispose , sinon vos objets résideront en mémoire pendant deux cycles de garbage (Remarque: lisez comment la finalisation fonctionne ). L'exemple ci-dessous explique tout ce qui précède.

public class DisposeExample
{
    // A base class that implements IDisposable. 
    // By implementing IDisposable, you are announcing that 
    // instances of this type allocate scarce resources. 
    public class MyResource: IDisposable
    {
        // Pointer to an external unmanaged resource. 
        private IntPtr handle;
        // Other managed resource this class uses. 
        private Component component = new Component();
        // Track whether Dispose has been called. 
        private bool disposed = false;

        // The class constructor. 
        public MyResource(IntPtr handle)
        {
            this.handle = handle;
        }

        // Implement IDisposable. 
        // Do not make this method virtual. 
        // A derived class should not be able to override this method. 
        public void Dispose()
        {
            Dispose(true);
            // This object will be cleaned up by the Dispose method. 
            // Therefore, you should call GC.SupressFinalize to 
            // take this object off the finalization queue 
            // and prevent finalization code for this object 
            // from executing a second time.
            GC.SuppressFinalize(this);
        }

        // Dispose(bool disposing) executes in two distinct scenarios. 
        // If disposing equals true, the method has been called directly 
        // or indirectly by a user's code. Managed and unmanaged resources 
        // can be disposed. 
        // If disposing equals false, the method has been called by the 
        // runtime from inside the finalizer and you should not reference 
        // other objects. Only unmanaged resources can be disposed. 
        protected virtual void Dispose(bool disposing)
        {
            // Check to see if Dispose has already been called. 
            if(!this.disposed)
            {
                // If disposing equals true, dispose all managed 
                // and unmanaged resources. 
                if(disposing)
                {
                    // Dispose managed resources.
                    component.Dispose();
                }

                // Call the appropriate methods to clean up 
                // unmanaged resources here. 
                // If disposing is false, 
                // only the following code is executed.
                CloseHandle(handle);
                handle = IntPtr.Zero;

                // Note disposing has been done.
                disposed = true;

            }
        }

        // Use interop to call the method necessary 
        // to clean up the unmanaged resource.
        [System.Runtime.InteropServices.DllImport("Kernel32")]
        private extern static Boolean CloseHandle(IntPtr handle);

        // Use C# destructor syntax for finalization code. 
        // This destructor will run only if the Dispose method 
        // does not get called. 
        // It gives your base class the opportunity to finalize. 
        // Do not provide destructors in types derived from this class.
        ~MyResource()
        {
            // Do not re-create Dispose clean-up code here. 
            // Calling Dispose(false) is optimal in terms of 
            // readability and maintainability.
            Dispose(false);
        }
    }
    public static void Main()
    {
        // Insert code here to create 
        // and use the MyResource object.
    }
}
CharithJ
la source
14

IDisposableexiste pour vous fournir un moyen de nettoyer les ressources non gérées qui ne seront pas nettoyées automatiquement par le garbage collector.

Toutes les ressources que vous «nettoyez» sont des ressources gérées et, en tant que telles, votre Disposeméthode ne fait rien. Votre classe ne devrait pas du tout implémenter IDisposable. Le garbage collector s'occupera très bien de tous ces champs tout seul.

Servy
la source
1
D'accord avec cela - il y a une notion de tout jeter quand ce n'est pas vraiment nécessaire. Dispose ne doit être utilisé que si vous avez des ressources non gérées à nettoyer !!
Chandramouleswaran Ravichandra
4
Ce n'est pas tout à fait vrai, la méthode Dispose vous permet également de disposer de "ressources gérées qui implémentent IDisposable"
Matt Wilko
@MattWilko Cela en fait un moyen indirect de se débarrasser des ressources non gérées, car cela permet à une autre ressource de se débarrasser d'une ressource non gérée. Ici, il n'y a même pas de référence indirecte à une ressource non gérée via une autre ressource gérée.
Servy
@MattWilko Dispose sera automatiquement appelé dans toute ressource gérée qui a implémenté IDesposable
panky sharma
@pankysharma Non, ce ne sera pas le cas. Il doit être appelé manuellement . C'est tout l'intérêt. Vous ne pouvez pas supposer qu'il sera automatiquement appelé, vous savez seulement que les gens sont censés l' appeler manuellement, mais les gens font des erreurs et oublient.
Servy
14

Vous devez utiliser le modèle jetable comme ceci:

private bool _disposed = false;

protected virtual void Dispose(bool disposing)
{
    if (!_disposed)
    {
        if (disposing)
        {
            // Dispose any managed objects
            // ...
        }

        // Now disposed of any unmanaged objects
        // ...

        _disposed = true;
    }
}

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

// Destructor
~YourClassName()
{
    Dispose(false);
}
Bélogix
la source
1
ne serait-il pas plus sage d'avoir un appel à GC.SuppressFinalize (this) dans le destructeur également? Sinon, l'objet lui-même serait récupéré dans le prochain GC
Sudhanshu Mishra
2
@dotnetguy: Le destructeur d'objets est appelé lorsque le gc s'exécute. Il n'est donc pas possible d'appeler deux fois. Voir ici: msdn.microsoft.com/en-us/library/ms244737.aspx
schoetbi
1
Alors maintenant, appelons-nous n'importe quel morceau de code standard un «modèle»?
Chel
4
@rdhs Non, nous ne le sommes pas. MSDN indique qu'il EST un modèle de « modèle Dispose » ici - msdn.microsoft.com/en-us/library/b1yfkh5e(v=vs.110).aspx donc avant vers le bas droit de vote peut - être Google un peu?
Belogix
2
Ni Microsoft ni votre publication n'indiquent clairement pourquoi le modèle devrait ressembler à ceci. Généralement, ce n'est même pas passe-partout, c'est juste superflu - remplacé par SafeHandle(et sous-types). Dans le cas de ressources gérées, la mise en œuvre d'une élimination appropriée devient beaucoup plus simple; vous pouvez réduire le code à une implémentation simple de la void Dispose()méthode.
BatteryBackupUnit
10

Vous n'avez pas besoin de faire votre Userclasse étant IDisposabledonné que la classe n'acquiert aucune ressource non gérée (fichier, connexion à la base de données, etc.). Habituellement, nous marquons les classes comme IDisposablesi elles avaient au moins un IDisposablechamp ou / et une propriété. Lors de la mise en œuvre IDisposable, mieux vaut le mettre selon le schéma typique de Microsoft:

public class User: IDisposable {
  ...
  protected virtual void Dispose(Boolean disposing) {
    if (disposing) {
      // There's no need to set zero empty values to fields 
      // id = 0;
      // name = String.Empty;
      // pass = String.Empty;

      //TODO: free your true resources here (usually IDisposable fields)
    }
  }

  public void Dispose() {
    Dispose(true);

    GC.SuppressFinalize(this);
  } 
}
Dmitry Bychenko
la source
C'est généralement le cas. Mais d'un autre côté, la construction using ouvre la possibilité d'écrire quelque chose qui ressemble à des pointeurs intelligents C ++, à savoir un objet qui restaure l'état précédent quelle que soit la façon dont un bloc using est quitté. La seule façon que j'ai trouvée de faire cela est de rendre un tel objet implémentable IDisposable. Il semble que je puisse ignorer l'avertissement du compilateur dans un cas d'utilisation aussi marginal.
Papa Schtroumpf
3

Idisposable est implémenté chaque fois que vous souhaitez un garbage collection déterministe (confirmé).

class Users : IDisposable
    {
        ~Users()
        {
            Dispose(false);
        }

        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
            // This method will remove current object from garbage collector's queue 
            // and stop calling finilize method twice 
        }    

        public void Dispose(bool disposer)
        {
            if (disposer)
            {
                // dispose the managed objects
            }
            // dispose the unmanaged objects
        }
    }

Lors de la création et de l'utilisation de la classe Users, utilisez le bloc "using" pour éviter d'appeler explicitement la méthode dispose:

using (Users _user = new Users())
            {
                // do user related work
            }

La fin de l'objet Users créé par le bloc using sera supprimée par un appel implicite de la méthode dispose.

S.Roshanth
la source
2

Je vois beaucoup d'exemples du modèle Microsoft Dispose qui est vraiment un anti-modèle. Comme beaucoup l'ont souligné, le code de la question ne nécessite pas du tout d'IDisposable. Mais si vous souhaitez l'implémenter, n'utilisez pas le modèle Microsoft. Une meilleure réponse serait de suivre les suggestions de cet article:

https://www.codeproject.com/Articles/29534/IDisposable-What-Your-Mother-Never-Told-You-About

La seule autre chose qui serait probablement utile est de supprimer cet avertissement d'analyse de code ... https://docs.microsoft.com/en-us/visualstudio/code-quality/in-source-suppression-overview?view=vs- 2017

MikeJ
la source