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.

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