0

Is this the right pattern to follow, i.e. if my class has disposable members I explicitly must call Dipose() on each one?

class MyClass : IDisposable
{
 public MyClass()
 {
  conn = maybeCreateAConnection();
 }
 public void Dispose()
 {
  if(conn!=null)conn.Dispose();
 }

 private SqlConnection conn;
}
Mr. Boy
  • 60,845
  • 93
  • 320
  • 589
  • 1
    [Dispose Pattern](https://msdn.microsoft.com/en-us/library/b1yfkh5e(v=vs.110).aspx), first **DO** - "**DO** implement the Basic Dispose Pattern on types containing instances of disposable types" – Damien_The_Unbeliever Apr 26 '16 at 13:38
  • 1
    I would challenge why you need a _member_ for the connection - connections should be created, used, and disposed of quickly. There is rarely a compelling reason to have a connection alive for the duration of a class. – D Stanley Apr 26 '16 at 13:38
  • @Damien_The_Unbeliever It seems kludgy, more like C++ having to remember which fields are disposable! – Mr. Boy Apr 26 '16 at 13:39
  • @Damien_The_Unbeliever the interesting question is probably what _containing_ means. I'm often confused too about that: just because I have a disposable field, am I really responsible for disposing it? What if this field can be set (via properties) from outside, how does the class user know that I will dispose an object he thought of as _his_ own? – René Vogt Apr 26 '16 at 13:40
  • @DStanley if I want to make multiple calls to the same DB in succession, isn't it better to maintain the connection rather than open and close it several times? In my case, the class itself is very short-lived. – Mr. Boy Apr 26 '16 at 13:41
  • 1
    Usually it doesn't really matter - the connections are pooled, and if you're releasing old connections properly, getting a new connection from the pool is rather trivial (basically just a Reset). – Luaan Apr 26 '16 at 13:43
  • @RenéVogt Whatever creates a disposable object is responsible for disposing of it. If it's a member and you don't know _when_ the client is done with it, then you should implement `IDisposable` so the client can tell you when they are done. Which is why disposable class members should be avoided if possible. – D Stanley Apr 26 '16 at 13:43
  • @Mr.Boy No, connections are pooled by .NET so creating them isn't generally an expensive process. If you call two commands in succession in the same _method_ then you can reuse the connection, but don't keep a connection alive for the entire life of the class. – D Stanley Apr 26 '16 at 13:44
  • @RenéVogt - having a settable disposable member exposed and claiming ownership opens a whole can of worms that is better avoided. – Damien_The_Unbeliever Apr 26 '16 at 13:46
  • @DStanley makes sense, thanks. My case is a bit complicated - there are multiple data sources and I have to find which one to use, then use the same one from then on. `MyClass` is only created for the time it takes to call several methods but I'll see if I can find a less ugly implementation :) – Mr. Boy Apr 26 '16 at 13:47
  • 1
    @Mr.Boy - persist the connection *string*, not the connection *object*. – Damien_The_Unbeliever Apr 26 '16 at 13:53
  • @Damien_The_Unbeliever yeah this occurred to me, might be easier. Thanks. – Mr. Boy Apr 26 '16 at 13:55
  • Or if you want to make things a bit less tight coupled, pass a `Func` instead of the connection string. Then you can just do something like `using (var conn = createConnection()) { ... }`. – Luaan Apr 26 '16 at 14:03

1 Answers1

3

Yes. But only if the class is the owner of said resource. You don't want to dispose of a connection that someone else is using.

And this has nothing to do with the Garbage Collector, by the way.

Luaan
  • 62,244
  • 7
  • 97
  • 116
  • How come? Remove the tag if you wish though it looks like it is a dupe. – Mr. Boy Apr 26 '16 at 13:38
  • 1
    @Mr.Boy `Dispose` is just a method like any other, it has no relation with the garbage collector. The GC only cares about references and finalizers. A finalizer *may* call a `Dispose` method, but that's irrelevant - it may also call any other method, as long as it's safe. – Luaan Apr 26 '16 at 13:40
  • The garbage collector is the reason .NET _has_ `IDisposable`. If we did not have managed memory and released all resources manually there would be no need for `IDisposable` - it would just go in the destructor (finalizer). – D Stanley Apr 26 '16 at 13:40
  • 1
    @DStanley No, not really. It's just a handy common interface for disposing of unmanaged resources, and that's all it's designed for. The finalizer will almost always call the `Dispose` method, but that's quite irrelevant. The `IDisposable` interface is also commonly used for disposing of managed resources (thanks to the handy `using` syntax), but that's not what it's intended for. – Luaan Apr 26 '16 at 13:41
  • I'm not sure that it should only dispose it if the class is the _owner_. There is a [BinaryReader](https://msdn.microsoft.com/en-us/library/2k01zeat(v=vs.110).aspx) (also BinaryWriter) that takes a stream. You've no chance to tell that it shoudn't take ownership but it does. In its Dispose it disposes the underlying stream as well and you can't avoid it. I'm with you that there should be a clear ownership and the owner has to dispose it but there are samples in .Net Framework that break that rule. – Sebastian Schumann Apr 26 '16 at 13:46
  • @Luaan The default finalizer will not call `Dispose`. You have to explicitly wire that up. The garbage collector will destroy objects _at some point_ after there are no references, which releases any _managed_ resources. Unmanaged resources must be explicitly released in the finalizer of the class that creates them. [The documentation for `IDisposable`](https://msdn.microsoft.com/en-us/library/system.idisposable(v=vs.110).aspx) mentions the garbage collector several times, so to say that it has "nothing to to with the garbage collector" is inaccurate. – D Stanley Apr 26 '16 at 13:48
  • This turned out way more contentious than I was expecting :) – Mr. Boy Apr 26 '16 at 13:49
  • @Verarind That's what the `leaveOpen` argument in the constructor is for. The default is true simply because that's how you use streams most often. – Luaan Apr 26 '16 at 13:59
  • @DStanley I never said it does. I'm not sure what point you're trying to make here - "The finalizer doesn't call Dispose on its own. But the garbage colelctor cares about the Dispose method." Eh? :) – Luaan Apr 26 '16 at 14:01
  • @Luaan I know but I'm currently using .Net 4.0 and in [that version](https://msdn.microsoft.com/en-us/library/system.io.binaryreader(v=vs.100).aspx) there is no `leaveOpen` parameter :-( – Sebastian Schumann Apr 27 '16 at 04:58
  • @Luaan Are you sure that *true* is the default? You may be right that this is the most often case but for backward compatibility it should be _false_. I found [this source](http://referencesource.microsoft.com/#mscorlib/system/io/binaryreader.cs) that shows that _false_ is the default value. – Sebastian Schumann Apr 27 '16 at 05:06
  • @Verarind Duh, of course not, my bad. I meant "it is disposed by default". Stupid inverted logic :D And you're right, .NET 4.0 doesn't seem to have it, which is a bit of an oversight. If you can't switch to a newer version, just make your own `Stream` that passes everything to the underlying stream and *ingores* the `Dispose` (or do the same thing with a custom `BinaryReader`). – Luaan Apr 27 '16 at 08:31