5

This question describes the same scenario as in CA2202, how to solve this case but it's not about how to fix the code, it's about why there's a problem in the first place.

The following code:

using (Stream stream = obtainStreamObject())
{
    using (var reader = new XmlTextReader(stream))
    {
        //do something with XmlTextReader
    }
}

causes Stream.Dispose() called twice. First the inner using block collapses which calls XmlTextReader.Dispose() which in turn causes Stream.Dispose(). Then the outer using block collapses and Stream.Dispose() is called again.

So I have to wrap Stream into using but not wrap XmlTextReader into using despite they both implement IDisposable.

This yields CA2202 warning

Do not dispose objects multiple times Object 'stream' can be disposed more than once in method 'MethodName(whatever)'. To avoid generating a System.ObjectDisposedException you should not call Dispose more than one time on an object.

Okay, it says that the second Dispose() might yield ObjectDisposedException. How does this make sense? Why would I implement Dispose() such that when it's called for the second time it would throw an exception instead of just doing nothing?

sharptooth
  • 167,383
  • 100
  • 513
  • 979
  • Does https://stackoverflow.com/a/32554589/34092 help? – mjwills Aug 11 '17 at 13:54
  • 1
    "Why would I implement `Dispose()` such that when it's called for the second time it would throw an exception instead of just doing nothing?" Because implementing it so it does nothing the second time is harder than not doing that. There is no formal requirement that `Dispose()` is safe to call multiple times. It is still very advisable to make `Dispose()` idempotent on disposed objects, and even implicitly thread-safe, but the contract doesn't require it. Hence *any* call on the object after `Dispose()` is technically unsafe -- even another `Dispose()`. – Jeroen Mostert Aug 11 '17 at 13:56
  • 3
    In this particular case, where objects own other objects that the analyzer knows nothing about and the calls are harmless, it's safe (and *advisable*) to [suppress the warning](https://stackoverflow.com/a/28456892/4137916), rather than jump through hoops to avoid it. – Jeroen Mostert Aug 11 '17 at 13:57
  • @mjwills Well, it does solve "the problem" but why do I have that "problem" in the first place? – sharptooth Aug 11 '17 at 14:00
  • Because while the guidelines encourage a second call to `Dispose` to do nothing, not everybody does this. Search for `bool disposed` in https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern . _That is what people should do, but often don't._ – mjwills Aug 11 '17 at 14:02
  • 3
    Also useful: https://blogs.msdn.microsoft.com/tilovell/2014/02/12/the-worst-code-analysis-rule-thats-recommended-ca2202/ – DavidG Aug 11 '17 at 14:23
  • 4
    Oh, per @DavidG's link, my comment was wrong: there *is* a formal requirement that calling it more than once is safe, because the documentation [says so](https://msdn.microsoft.com/library/system.idisposable.dispose): "If an object's `Dispose` method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its `Dispose` method is called multiple times." This has been there all the way back to .NET 1.1, so code that isn't "multiple-`Dispose`-safe" is just broken, making CA2202 a bad rule indeed. (Thread safety is still optional.) – Jeroen Mostert Aug 11 '17 at 14:36
  • 1
    @JeroenMostert The documentation *specifically* says that a disposable object should not throw if disposed multiple times. Sure, the compiler can't enforce it anywhere, but it *is* a part of the formal contract of `IDisposable` that you're *expected* to follow, despite it being unenforcable. – Servy Aug 11 '17 at 15:30
  • @Servy: hence the follow-up comment where I call myself out... I didn't feel like retyping things. – Jeroen Mostert Aug 11 '17 at 15:32
  • 1
    As has been amply discussed already (why do the highest-voted questions so often seem the most poorly-researched?), the warning is not about how _you'd_ implement `IDispose`, but how someone else might. They aren't _supposed_ to, but they might. Whether you want to worry about that is up to you. Most people (myself included) don't. We trust that good code won't throw `ObjectDisposedException` when calling dispose, and we work hard to not use bad code. – Peter Duniho Aug 11 '17 at 23:03
  • 1
    Do note, it's somewhat more subtle than the immediate object throwing; one might naively implement `IDisposable` in a way that attempts to access owned objects that were disposed by the previous call to `Dispose()`. Again, you're not _supposed_ to implement `IDisposable` that way, but it's an easy trap to fall into if you're not thinking. – Peter Duniho Aug 11 '17 at 23:03

0 Answers0