BinaryReader throws NullReferenceException when being disposed

Question

I have a class MyClass that needs data from a file that will be used throughout the run of the program. To read in the data I have another class OpenFileService that derives from IDisposable and uses a BinaryReader to read in all the data:

internal class OpenFileService : IDisposable
{
    #region disposing data
    bool disposed = false;

    public void Dispose()
    {
        if (!disposed)
        {
            Dispose(true);
            GC.SuppressFinalize(this);
            disposed = true;
        }
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            br.Dispose();
        }
    }

    ~OpenFileService()
    {
        Dispose(false);
    }
    #endregion

    internal event EventHandler ErrorInFileReadEventHandler;
    internal event EventHandler SuccessfulFileReadEventHandler;

    private BinaryReader br;

    internal void OpenFile(object obj)
    {
        MyClass sender = obj as MyClass;
        bool isWrongFormat = false;

        try
        {
            br = new BinaryReader(File.Open((sender).fileName, FileMode.Open));
            //read in header from file.
            if (//header shows wrong file format)
            {     
                isWrongFormat = true;
                throw new System.Exception();
            }
            //read rest of file.
            SuccessfulFileReadEventHandler(this,null);
        }
        catch
        {
            if (isWrongFormat)
                MessageBox.Show("Wrong format.");
            else
                MessageBox.Show("Couldn't access.");
            ErrorInFileReadEventHandler(this, null);
            return;
        }
        finally
        {
            this.Dispose();
        }
    }
}

And it is used as such:

class MyClass
{
    internal filePath; //assuming it has already been determined

    internal ImageData(string filePath)
    {
        this.filePath = filePath;
        OpenFileService ofs = new OpenFileService();
        ofs.ErrorInFileReadEventHandler += new EventHandler(UnsuccessfulFileRead);
        ofs.SuccessfulFileReadEventHandler += new EventHandler(SuccessfulFileRead);
        Thread thread = new Thread(new ParameterizedThreadStart(ofs.OpenFile));
        thread.IsBackground = true;
        thread.Start(this);
    }
}

Now when the file format is wrong and I create the exception myself in the try block, everything works without any problems, but when the file could actually not be accessed (e.g. write-protected) br.Dispose(); creates a NullReferenceException and I cannot figure out why. I really stripped down the code to its bare essentials, I hope it's not too long.

Edit: Possible answer can be found from the accepted answer here as a recommended answer by Microsoft.

Wow...I actually missed that thought. Replacing `br.Dispose();` with `if(br != null) br.Dispose();` seems to solve it... Thank's a lot! I can't believe how I didn't think of this. — philkark, Sep 15 '13 at 15:22

Douglas · Accepted Answer · 2013-09-15T15:49:43.470

1

The issue might become clearer if you split your file-open logic across two lines:

try
{
    var fs = File.Open((sender).fileName, FileMode.Open);
    br = new BinaryReader(fs);
}
finally
{
    br.Dispose();
}

When the File.Open call fails, the exception is thrown without anything being assigned to the br field. When you attempt to dispose it in the finally block, it would still be null, thus your exception.

Edit: The way I would suggest fixing this:

try
{
    using (FileStream fs = File.Open(sender.fileName, FileMode.Open))
    using (BinaryReader br = new BinaryReader(fs))
    {        
        //read in header from file.
        if ( /*header shows wrong file format*/ )
        {     
            isWrongFormat = true;
            MessageBox.Show("Wrong format.");
            ErrorInFileReadEventHandler(this, null);
        }
        else
        {
            //read rest of file.
            SuccessfulFileReadEventHandler(this, null);
        }
    }
}
catch
{
    MessageBox.Show("Couldn't access.");
    ErrorInFileReadEventHandler(this, null);
}

In the process, I've demoted your BinaryReader from a field to a local variable. Since you're only accessing it within the OpenFile method (and disposing it before returning), there's no need for its reference to persist outside the method.

edited Sep 15 '13 at 15:49

answered Sep 15 '13 at 15:24

Douglas

53,759
13
140
188

I accept this answer, because I realized that was the problem after the comment on my question. Thank you for your answer, I will change it. Do I not have to dispose fs as an unmanaged resource? – philkark Sep 15 '13 at 15:26
In general, you should dispose of any `IDisposable` object that you instantiate. However, file-handling wrappers are an (unfortunate) exception. The `Dispose` method of the `BinaryReader` class will also close any stream that you passed to its constructor during initialization. So, the short answer is no, you don't have to dispose the file stream as long as you dispose the wrapping `BinaryReader`. – Douglas Sep 15 '13 at 15:29
Okay, thank you, I had the feeling Microsoft would add an confusing exception to their disposing conventions. – philkark Sep 15 '13 at 15:30
Refer to this answer (and ensuing discussion) if you would like an elaboration: [Does disposing streamreader close the stream?](http://stackoverflow.com/a/1065196/1149773) – Douglas Sep 15 '13 at 15:33
I just realized that your suggested answer might not work. If fs throws the exception, `br.Dispose();` will be called without having initialized it. – philkark Sep 15 '13 at 15:34
I didn't mean for my code snippet to be a suggested answer, but just a minor alteration of your code to make the issue more apparent. I'll add the suggested answer to avoid confusion. – Douglas Sep 15 '13 at 15:36
Thank you. I would keep it as an accepted answer in any case, but to avoid confusion, it would be great if you change it. Thanks for the link as well. – philkark Sep 15 '13 at 15:37
I would also like to refer to this answer: http://stackoverflow.com/questions/12000136/using-statement-filestream-and-or-streamreader-visual-studio-2012-warnings there seems to be a recommended way by Microsoft. – philkark Sep 15 '13 at 15:54
I personally wouldn't care for that pattern. Microsoft have been "recommending" some terrible code practices recently. As correctly pointed out in a [subsequent answer](http://stackoverflow.com/a/12022068/1149773), it should be safe to assume that `Dispose` is idempotent (i.e. can be called multiple times consecutively). – Douglas Sep 15 '13 at 16:02

BinaryReader throws NullReferenceException when being disposed

1 Answers1