CA2000 when Returning Disposable Object from Method

Question

I have a factory method that builds objects that implement IDisposable. Ultimately it is the callers that manage the lifetime of the created objects. This design is triggering a bunch of CA2000 errors. Is there something fundamentally incorrect in my design, does it need refactoring, or is it just getting too excited about static code analysis warnings?

The factory method

public static DisposableType BuildTheDisposableType(string param1, int param2)
{
    var theDisposable = new DisposableType();

    // Do some work to setup theDisposable

    return theDisposable
}

A caller

using(var dt = FactoryClass.BuildTheDisposableType("data", 4))
{
   // use dt
}

This question should be fine here, but in the future, it might be better suited on [programmers.stackexchange.com](http://programmers.stackexchange.com/). — gunr2171, Dec 06 '13 at 20:10
@gunr2171: I disagree. This is a perfectly good question for [so]. — John Saunders, Dec 06 '13 at 20:11
Note that if an exception occurs between the creation of your variable and the return statement, the disposable will leak. I would suggest including a `finally` block which checks if `theDisposable` is non-null and invokes `theDisposable.Dispose()` if so. To return the object, copy the reference to another variable and null out the original reference. Too bad there's no "keep using" statement to cancel the effects of `using` on select program paths. — supercat, Dec 11 '13 at 18:21
@supercat it wouldn't make sense to dispose of an object that is meant to be returned. — Crono, Sep 24 '14 at 21:19
@Crono: Once the method passes the last point that could plausibly throw an exception (before the `finally` block) it should copy the thing that's going to be returned to another variable, null out the original, and return that other variable. When the `finally` block runs, it won't have anything to `Dispose`. The only time `Dispose` will get called is if an exception is thrown. In that scenario, the caller isn't going to receive a reference to the object and thus won't be able to dispose it. If the method that created the object doesn't dispose it, nothing will. — supercat, Sep 24 '14 at 22:32
@supercat Ah, you're right. Somehow my brain erased the part about copying the reference. I see it now. :) I just tried it on my side and although the CA will still issue a warning over it, it's still an interesting technique I'll keep into mind. Thanks. — Crono, Sep 25 '14 at 12:05

score 20 · Answer 1 · edited Nov 07 '15 at 20:50

20

You should store it to local variable, and wrap initialization in the try-catch-rethrow block, dispose in case of any exception:

public MyDisposable CreateDisposable()
{
    var myDisposable = new MyDisposable();
    try
    {
        // Additional initialization here which may throw exceptions.
        ThrowException();
    }
    catch
    {
        // If an exception occurred, then this is the last chance to
        // dispose before the object goes out of scope.
        myDisposable.Dispose();
        throw;
    }
    return myDisposable;
}

Try to never leave the disposable object vulnerable to exceptions, when Dispose wouldn't be called

PS: someone previously mentioned to dispose inside the finally - this is obviously wrong - in non-exception path you don't want to call Dispose

edited Nov 07 '15 at 20:50

Wim Coenen

66,094
13
157
251

answered Aug 21 '15 at 00:17

Mikl X

1,199
11
17

What is the execution path where it is not disposed? – John Saunders Nov 06 '15 at 11:08
1

@JohnSaunders: in the OP's original question, when an exception is thrown in the step indicated by "// Do some work to setup theDisposable". At that point a disposable object has been created, but all references to it will go out of scope and the caller will only get the exception (instead of a return value) so the disposable object is left hanging. – Wim Coenen Nov 07 '15 at 20:40
What if `new MyDisposable()` throws an exception? How about putting the call to the constructor within the `try` block? And then doing a `null` test on `myDisposable` before calling `myDisposable.Dispose()` in the `catch` block. I found this cleared CA2000 for me. – DavidRR Mar 08 '16 at 21:26
3

DavidRR, that wouldn't make any difference. In case of exception in new, the assignment will not be called, i.e. myDisposable will not be changed - thus nothing to release. – Mikl X Mar 09 '16 at 06:40
Sorry, but how does this (albeit fundamentally correct) solve the CA2000 issue? It would still throw the warning/error, and that's the core of the question. – String.Empty Oct 13 '22 at 21:27
2

@String.Empty The example in this answer should not result in a CA2000 issue. If you are still seeing CA2000 after following this pattern, then something else must be at play. In that case, it would probably best to post a new stack overflow question with some more details about your code. – Wim Coenen Oct 16 '22 at 11:06
It still throws the CA2000 on .net 7.0.203 SDK – Mark Adamson Apr 20 '23 at 08:06

score 10 · Accepted Answer · answered Dec 06 '13 at 20:17

10

I would recommend that you suppress the CA2000 warning on each individual factory method, or perhaps on the entire class that contains them (but only if that is the only function of that class).

I further recommend that you include a justification:

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability",
    "CA2000:Dispose objects before losing scope",
    Justification = "This is a factory method. Caller must dispose")]

answered Dec 06 '13 at 20:17

John Saunders

160,644
26
247
397

7

CA2000 actually doesn't have a problem with methods returning disposable objects and the caller taking ownership. It is trying to tell us here that there is an execution path where the disposable object will not be handed off properly to the caller, so **this is not a false positive - do not suppress**! @MiklX posted the right answer, I've cleaned it up a bit to clarify. – Wim Coenen Nov 06 '15 at 09:53
@WimCoenen perhaps that was the case back in 2015, but in 2022 that exact piece of code is throwing CA2000 warnings/errors. – String.Empty Oct 13 '22 at 21:32

score 2 · Answer 3 · answered Dec 06 '13 at 20:10

2

You're getting the error because the creator of the disposable object isn't managing it. However, there's nothing fundamentally wrong with the design. You are just relying on the consumers to leverage using. Not much different than the current ADO objects for example.

answered Dec 06 '13 at 20:10

Mike Perrenoud

66,820
29
157
232

Passing responsibility is a means of managing it; the problem is that .NET provides no means of distinguishing a method that returns a reference to an `IDisposable` owned by someone else, from one which conveys ownership of a returned `IDisposable` to the caller, and thus code-diagnostic tools have no reliable way of knowing whether they should squawk when a method returns an `IDisposable` without disposing it, or when a method which returns an `IDisposable` is called and its return value is abandoned. – supercat Jun 11 '14 at 16:01

score 0 · Answer 4 · answered Jun 06 '14 at 19:49

Another alternative is to change your factory method into a "configuration" method and to put the responsibility of creating the disposable object onto the client. Example:

public void SetupDisosableThing(IDisposable foo)
{
 foo.Bar = "baz";
}

void Main()
{
  using (var x = new Thing())
  {
   SetupDisposableThing(x);
  }
}

CA2000 when Returning Disposable Object from Method

4 Answers4