43

I have a method that takes FileStream as input. This method is running inside a for loop.

private void UploadFile(FileStream fileStream)
{
    var stream = GetFileStream();
    // do things with stream
}

I have another method which creates and returns the FileStream:

private FileStream GetFileStream()
{
    using(FileStream fileStream = File.Open(myFile, FileMode.Open))
    {
        //Do something
        return fileStream;
    }
}

Now the first method throws an ObjectDisposedException when I try to access the returned FileStream, probably because it is already closed since I am using "using" to properly dispose the stream.

If I don't use "using" and instead use it as follows, then the FileStream remains open and the next iteration of the loop (operating on the same file) throws an exception telling the file is already in use:

private FileStream GetFileStream()
{
    FileStream fileStream = File.Open(myFile, FileMode.Open);
    //Do something
    return fileStream;
}

If I use a try-finally block, where I close the stream in the finally then it also throws the ObjectDisposedException.

How to effectively return file stream and close it?

Douglas
  • 53,759
  • 13
  • 140
  • 188
Frank Martin
  • 3,147
  • 16
  • 52
  • 73
  • 5
    You can't close it, not your job. Use good names. "Get" is not enough to help the programmer figure out that he needs to dispose the stream, use "Create" instead. – Hans Passant Aug 17 '15 at 20:01

4 Answers4

49

When you return an IDisposable from a method, you are relegating the responsibility of disposing it to your caller. Thus, you need to declare your using block around the entire usage of the stream, which in your case presumably spans the UploadFile call.

using (var s = GetFileStream())
    UploadFile(s);
Douglas
  • 53,759
  • 13
  • 140
  • 188
23

The problem is that the FileStream object is disposed as soon as you exit from the GetFileStream() method, leaving it in an unusable state. As other answers already indicate, you need to remove the using block from that method and instead put the using block around any code that calls this method:

private FileStream GetFileStream()
{
    FileStream fileStream = File.Open(myFile, FileMode.Open);
    //Do something
    return fileStream;
}

using (var stream = GetFileStream())
{
    UploadFile(stream);
}

However, I want to take this a step further. You want a way to protect the stream created by your GetFileStream() from the case where a sloppy programmer might call the method without a using block, or at least somehow strongly indicate to callers that the result of this method needs to be enclosed with a using block. Therefore, I recommend this:

public class FileIO : IDisposable
{
    private FileStream streamResult = null;

    public FileStream CreateFileStream(string myFile)
    {
        streamResult = File.Open(myFile, FileMode.Open);
        //Do something
        return streamResult;
    }

    public void Dispose()
    { 
       if (streamResult != null) streamResult.Dispose();         
    }

}

using (var io = FileIO())
{
    var stream = io.CreateFileStream(myFile);

    // loop goes here.
}

Note that you don't necessarily need to create a whole new class for this. You may already have an appropriate class for this method where you can just add the IDisposable code. The main thing is that you can use IDisposable as a signal to other programmers that this code should be wrapped with a using block.

Additionally, this sets you up to modify the class so that you could create your IDisposable object once, before the loop, and have the new class instance keep track of everything you need to dispose at the end of the loop.

Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
  • 1
    This is a great answer! @Joel would you mind elaborating a little bit on how `IDisposable` is used as a signal to other programmers? How do other programmers know when instantiating this class that they should wrap their code into a `using` block? – jrn Feb 15 '17 at 16:00
  • 4
    How does this help? The `Stream` class is already `IDisposable`. Furthermore, there is nothing preventing from calling `io.CreateFileStream(..)` multiple times within the `using(var io = FileIO())` block. – oɔɯǝɹ Sep 07 '19 at 12:27
5

If you have a method that need to return an open file stream then all callers of that method need to take responsibility for disposing of the returned stream, since it cannot dispose of the stream before returning it.

Servy
  • 202,030
  • 26
  • 332
  • 449
-2

I am not sure how to read the code in the question, since the UploadFile method receives fileStream, but then creates its own stream via GetFileStream and does not use fileStream at all.

But still I have a suggestion that might solve similar problems, too. It called the 'Factory Isolation Pattern' (from the book 'Adaptive Code via C#' by Gary McLean Hall) The idea is to keep object creation and destruction together, but still allow using the object in a flexible way. And all it takes is a little variation of @frankmartin's original GetFileStream method, only we turn things around and instead of letting the disposable object escape, we let the "do something" in:

private void With(Action<FileStream> do)
{
    using (FileStream fileStream = File.Open(myFile, FileMode.Open))
    {
        do(fileStream);
    }
}

You can then use this method in this way:

With(fileStream => UploadFile(fileStream);

Here the user cannot forget to dispose the stream (as @oɔɯǝɹ pointed out), in fact the user does even need know that it has to be disposed or taken care of in any special way ...

AlWo23
  • 72
  • 3