CODEWITHSUNDEEP
c#streamfilestreamusingidisposable
Frank Martin
Frank Martin

Reputation: 3437

How to return a Stream from a method, knowing it should be disposed?

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 can I effectively return FileStream and close it?

Upvotes: 48

Views: 36196

Answers (4)

Joel Coehoorn
Joel Coehoorn

Reputation: 415600

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 = new 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.

Upvotes: 24

AlWo23
AlWo23

Reputation: 72

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 ...

Upvotes: -2

Douglas
Douglas

Reputation: 54877

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);

Upvotes: 54

Servy
Servy

Reputation: 203802

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.

Upvotes: 5

Related Questions