Best way to return IDisposable object and only disposing on Exception?

Question

I've been searching for answer on this but I couldn't really find the exact case I'm implementing. I want to return IDisposable object within a method without Disposing/Closing in the same method. But I want to dispose the object only when Exception is raised. What would be the best way to handle in this case? Following is the current way I'm handling this case.

public DisposableObject GetObject()
{
   DisposableObject object = null;

   try
   {
      object = new DisposableObject();
      object.Open();
   }
   catch
   {
      if (object != null)
      {
         object.Close();
      }
      throw;
   }
   return object;
}

The object created from above method will be consumed and disposed by other method like this.

public void OtherMethod()
{
  using(var object2 = GetObject())
  {
    DoSomethingWithObject2(object2);
  }
}

Is there any other option here in this case?

Answer 1

Like Morton said, the issue is with calling Open on the DisposableObject instance within the factory method, but I don't agree with the corrosponding solution.

The real problem is that Open is a leaky abstraction. Why would I want to instantiate an invalid instance? When I want a file stream, I call File.Open(...), I don't instantiate a new Stream and call OpenFile(...). That would force me to know how to open the file, and defeat the purpose of the factory method alltogether (yes, File.Open is a factory method for a Stream instance).

So when I read a file, I read it like this:

using (var stream = File.Open("myFile.text", FileMode.Open))
using (var reader = new StreamReader(stream))
{
    var content = reader.ReadToEnd();

    Console.WriteLine(content);
}

I don't have to worry about how to close the file if the file couldn't be opened properly. In fact, If the Open call throws an exception, the using statement can't even call Dispose() on stream, because stream hasn't been assigned yet and execution flow hasn't even entered the using statement's block scope yet.

public class Program
{
    public void Main()
    {
        try
        {
            using (var obj = MyObj.Create())
            {
                // everything is already in a valid state.
                var message = obj.ReadMessage();

                Console.WriteLine(message);
            }
        }
        catch (Exception ex)
        {
            // If the factory succeeded, the using statement did the cleanup.
            // if the factory failed, the factory took care of the cleanup.
            Console.WriteLine(ex.Message);
        }
    }
}

and

public class MyObj : IDisposable
{
    public static MyObj Create()
    {
        Stream stream = null;
        StreamReader reader = null;

        try
        {
            stream = File.Open("myFile.txt", FileMode.Create);
            reader = new StreamReader(stream);

            return new MyObj(reader);
        }
        catch
        {
            reader?.Dispose();
            stream?.Dispose();

            throw;
        }
    }

    private readonly StreamReader _reader;

    private MyObj(StreamReader reader)
    {
        _reader = reader;
    }

    public string ReadMessage()
    {
        return "the message: " + _reader.ReadToEnd();
    }

    public void Dispose()
    {
        _reader?.Dispose();
    }
}

Answer 2

I think I'd rather push all responsibility of openening and closing the connnection to the user of this object instead of the factory.

The current method you have shown, one cannot create an instance, e.g. testing, without creating the actual connection. Furthermore, is the method not transparent, as another person would not assume the connection to be opened.

The method I've posted enables the user of your code to easier handle the error which may occure from opening the DisposableObject.

    public DisposableObject GetObject() {
        return new DisposableObject();
    }

    public void OtherMethod() {
        using (DisposableObject o = GetObject()) {
            try {
                o.Open();
            } catch (Exception ex) {
                // Log(ex);
            } finally {
                // If not called within the dispose function.
                o.Close();
            }
        }
    }

Though, if you would like to handle the opening, your method is alright. However, I would change some minor details:

    public DisposableObject GetObject() {
        var o = new DisposableObject();
        try {                
            o.Open();
            return o;
        } catch (Exception ex) {
            o.Dispose(); // o.Close is called within.
            throw ex;
        } 
    }