Saving a Class to disk on dispose: Does my code have bugs?

Question

I am attempting to make a simple class that serializes itself to disk when it is no longer in use. The code I have right now (see below). The code I have now seems to work, but I am not fully confident in my knowledge, so I am wondering if anyone else sees any significant problems with this code.

void IDisposable.Dispose()
{
    Dispose(true);
    GC.SuppressFinalize(this);
}

~MyClass()
{
    Dispose(false);
}

protected virtual void Dispose(bool disposing)
{
    if (!this.disposed)
    {
        MemoryStream ms = new MemoryStream();
        BinaryFormatter bf = new BinaryFormatter();
        bf.Serialize(ms, this);
        byte[] output = Dostuff(ms);
        File.WriteAllBytes(DBPATH, output);
    }
    this.disposed = true;
}

Answer 1

This will likely work - but I wouldn't do it. By doing this, you're potentially executing potentially dangerous code in the finalization thread. If anything goes wrong, you'll be in bad shape....

Dispose should really do nothing but dispose of your resources. I would strongly recommend moving this to another method, and making it part of the object's API instead of relying on IDisposable to handle processing for you.

Answer 2

It is virtually impossible to write a finalizer correctly, and doing this sort of work in one is just a recipe for disaster. Not to mention it'll kill performance and be impossible to debug. Rule 1 for finalizers is don't ever use them. Rule 2 (for advanced users only) is don't use them unless you're really sure you have to.

If it's just for a fun hobby project then there's no real harm and it will probably work well enough, but I'd cry if I ever saw something like this in a production codebase.

If you do want to do something like this, then I'd make it an explicit call and simply use the finalizer to catch cases during debugging where the explicit method was not called, e.g.

class MyClass
{
    private bool dirty; // set this whenever the object changes

    ~MyClass 
    {
        if (this.dirty) 
        {
            Debug.Fail("Object was not saved.");
        }
    }

    public void Save()
    {
        if (this.dirty)
        {
            // TODO: do the save
            this.dirty = false;
        }
    }
}

Answer 3

First of all, I think you have pretty weak design, because your class violate Single Responsibility Principle. Much more preferable distinguish two responsibilities: serializable entity and saving/reading this entity to/from persistent storage. In most cases serializable entities are lightwait and finalizable classes are not.

Second, you should avoid complex logic inside your finalizers. For example, it would be much better save your serializable class into persistent storage into Storage.Dispose method. And from finalizer method only write warning into log file because it shows inappropriate class Storage usage:

[Serializable]
public class MySerializableClass {
}

public sealed class MyStorage : IDisposable {

  ~MyStorage()
  {
     Dispose(false);
  }


  public void Dispose()
  {
     Dispose(true);
     GC.SuppressFinalize(this);
  }


  void Dispose(bool disposing)
  {
     if (!this.disposed)
     {
        if (disposing)
        {
          //We can access to all managed resources
          using (var ms = new MemoryStream())
          {
            BinaryFormatter bf = new BinaryFormatter();
            bf.Serialize(ms, mySerializableClass);
            byte[] output = Dostuff(ms);
            File.WriteAllBytes(DBPATH, output);
          }
        }
        else
        {
           //Inappropriate storage usage!
           //We can't guarantee that mySerializableClass object would
           //properly saved to persistant storage.
           //Write warning to log-file. We should fix our code
           //and add appropriate usage!
        }
    }
    this.disposed = true;
 }

}