Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This code was posted as answer to Async file writer in .Net 3.5, My question is how would it be done better using Tasks or new features in .Net 4.5?

public sealed class Logger : IDisposable
{
    private delegate void WriteMessage(string message);
    private static readonly Logger Instance = new Logger();
    private static object Locker = new object();
    private static readonly StreamWriter Writer =
                        new StreamWriter("c:\\temp\\logtester.log", true);
    private static bool Disposed;

    private Logger()
    {
    }

    ~Logger()
    {
        Dispose(false);
    }

    public static void Log(string message)
    {
        WriteMessage action = Instance.MessageWriter;
        action.BeginInvoke(message, MessageWriteComplete, action);
    }

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

    private static void Dispose(bool disposing)
    {
        lock (Locker)
        {
            if (Disposed)
            {
                return;
            }

            if (disposing)
            {
                if (Writer != null)
                {
                    Writer.Dispose();
                }
            }

            Disposed = true;
        }
    }

    private static void MessageWriteComplete(IAsyncResult iar)
    {
        ((WriteMessage)iar.AsyncState).EndInvoke(iar);
    }

    private void MessageWriter(string message)
    {
        lock (Locker)
        {
            Writer.WriteLine(message);
        }
    }
}
share|improve this question

1 Answer

up vote 10 down vote accepted

Before I explain how would I improve that code, I'll first mention two problems I will not try to fix (because I'm not sure how to do it properly):

  1. Disposable singleton is a really weird combination. If something should be available anytime and anywhere, it should be a singleton. On the other hand, if something has a limited lifetime and holds an expensive resource (like an open file), it should be disposable. Those two don't go together very well.

  2. Finalizer should execute as fast as possible. It certainly shouldn't take a lock that may not be released for quite some time (because there may be many threads also holding the lock).

The problems my solution will solve:

  1. Writing to the log may not be in the correct order, lock doesn't guarantee any ordering.
  2. Dispose() may not write all outstanding messages. This is again because lock doesn't guarantee ordering.
  3. If a write attempt happens after Dispose() (either because Log() was called after Dispose(), or because of the ordering problems mentioned above), an exception will be thrown on the ThreadPool. This exception can't be caught from outside of Logger and will bring down the whole application. (More reason not to have disposable singletons.)
  4. If there are many calls to Log() at the same time, or if the writing takes too long, there may be many threads, all waiting on the same lock.
  5. While the message is being written, there is a thread blocking until it finishes.

My solution using TPL Dataflow (which uses .Net 4.5 extensively, available from NuGet) solves the issues so that:

  1. Writing to the log will be always in the correct order.
  2. Dispose() will first write all outstanding messages before returning.
  3. If Log() is called after Dispose(), it will directly throw an exception.
  4. There will be at most one thread doing the writing, at any time.
  5. While the message is being written, no thread is waiting for it to finish.
public sealed class Logger : IDisposable
{
    private delegate void WriteMessage(string message);

    private static readonly Logger Instance = new Logger();

    private readonly ActionBlock<string> m_writerBlock;

    private static bool Disposed;

    private Logger()
    {
        var writer = new StreamWriter("c:\\temp\\logtester.log", true);
        m_writerBlock = new ActionBlock<string>(s => writer.WriteLineAsync(s));
        m_writerBlock.Completion.ContinueWith(_ => writer.Dispose());
    }

    ~Logger()
    {
        Dispose(false);
    }

    public static void Log(string message)
    {
        if (!Instance.m_writerBlock.Post(message))
            throw new ObjectDisposedException("Logger");
    }

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

    private void Dispose(bool disposing)
    {
        if (Disposed)
            return;

        if (disposing)
        {
            m_writerBlock.Complete();
            m_writerBlock.Completion.Wait();
        }

        Disposed = true;
    }
}

This works correctly, because the ActionBlock will take care of almost anything needed here, like ordering, synchronization and scheduling Tasks when necessary. WriteLineAsync() returns a Task and the block will use this Task to asynchronously wait for it to finish.

share|improve this answer
Very nice, I would upvote, but I don't yet have the reputation (almost there). I was not aware of the ActionBlock, I briefly looked at the MSDN entry for it and I'm curious how this will preserve ordering? Is it due to the "degree of parallelism"? – jeremywho Jul 3 '12 at 18:06
ActionBlock maintains a queue that takes care of the ordering. Because of that, it preserves the order unless you specify MaxDegreeOfParallelism other than 1. But you certainly don't want to do that here. – svick Jul 4 '12 at 8:24

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.