CODEWITHSUNDEEP
c#.netgarbage-collection
houen
houen

Reputation: 941

Suppressing premature finalization of stream in .NET

I have the following logger-like class pattern:

public class DisposableClassWithStream : IDisposable
{
    public DisposableClassWithStream()
    {
        stream = new FileStream("/tmp/file", FileMode.Create, FileAccess.ReadWrite, FileShare.ReadWrite);
        writer = new StreamWriter(stream);
    }

    public void WriteLine(string s)
    {
        writer.WriteLine(s);
    }

    ~DisposableClassWithStream()
    {
        Dispose();
    }

    private readonly object disposableLock = new object();
    private bool isDisposed;

    public void Dispose()
    {
        lock (disposableLock)
        {
            if (!isDisposed)
            {
                writer.Close();
                stream.Close();
                GC.SuppressFinalize(this);
                isDisposed = true;
            }
        }
    }

    private FileStream stream;
    private TextWriter writer;
}

and a very simple code using this class:

public static void Main(string[] args)
{
    var t = new DisposableClassWithStream();
    t.WriteLine("Test");
}

The code throws (nondeterministically), both on .net and mono, ObjectDisposedException caused by method Close of object writer as it tries to flush buffer to stream which is already disposed.

I understand that the reason is that GC finalizes stream before writer. How can I change the class pattern to be sure that stream is not disposed before the writer?

I tired to use GC.SuppressFinalize(writer) in the constructor, but I'm not sure if it is not too hacky.

Edit:

I would like to address the question of having finalizer in the first place. As mentioned at the beginning of the question, the class is used as a logger and I want to assure that all lines from writer are flushed to the hard disk before closing the process.

Upvotes: 1

Views: 518

Answers (3)

supercat
supercat

Reputation: 81159

When a finalizer runs, most managed objects to which it holds references will meet one of the following criteria:

-1- Not care about cleanup, in which case the finalizer should do nothing with them.

-2- Be unable to perform cleanup in a thread-safe manner when invoked in the context of finalizer cleanup, in which case the finalizer should do nothing with them.

-3- Have already cleaned themselves up using their own finalizer, in which case the finalizer should do nothing with them.

-4- Have a Finalize method which hasn't run yet, but is scheduled to run at first opportunity, in which case the finalizer for the class holding the reference should do nothing with them.

Different objects will meet different criteria, and it may be difficult to know which of those criteria a particular object might meet, but for the vast majority of objects will meet any of the criteria, the required handling is the same: don't do anything with them in the finalizer.

There are a few rare scenarios involving things like weak events where finalizers may be able to do something useful with managed objects, but in nearly all such cases, the only classes which should have finalizers are those whose sole purpose is to managed the finalization cleanup of other intimately-related objects. If one doesn't understand all the wrinkles of finalization, including the differences between short and long weak references and how they interact with the finalizer, any finalizer one tries to write will likely do more harm than good.

Upvotes: 0

Dennis
Dennis

Reputation: 37770

Do not create finalizer, unless your IDisposable implementation really works with unmanaged resources. FileStream and StreamWriter are managed resources.

Moreover, since SafeHandle was introduced, it is hard to imagine use-case, when any unmanaged resource couldn't be wrapped into managed SafeHandle. Do not follow blindly IDisposable implemetation from MSDN - it's OK, but it is intended for the case, when your type operates both managed and unmanaged resources.

Remove finalizer at all, and throw away GC.SuppressFinalize(this); from Dispose:

public void Dispose()
{
    lock (disposableLock)
    {
        if (!isDisposed)
        {
            writer.Close();
            stream.Close();
            isDisposed = true;
        }
    }
}

Update.

Finalizers are intended for unmanaged resource cleanup.
You may consider finalizer as a place to close file handle, network socket, etc. This isn't a place for any application logic. In general, managed object during finalization resides in unusable state - there's no guarantee, that any of its IDisposables (like stream or writer in your sample) are not finalized already.

If you want to be sure, that particular log message is flushed into file, than write it and call writer.Flush(). Otherwise, if immediate flushing is undesirable for you, make sure, that you're calling dispose for the logger on application shutdown. Also note, that you can't protect from process termination, so don't be too paranoid with your logger.

Upvotes: 4

MarkO
MarkO

Reputation: 2233

Managed objects should not be cleaned up in the finalizer. The finalizer should only used for cleaning up unmanaged resources.

Rewrite your code as follows to dispose of managed resources when you are done with the streams.

public static void Main(string[] args)
{
    using (var t = new DisposableClassWithStream())
    {
    t.WriteLine("Test");
    }
}

And be sure to check out the Dispose Pattern article on MSDN

Upvotes: 1

Related Questions