3
\$\begingroup\$

My question is about an easy FileLogger. I want the FileLoggers to store new logs in a collection, and write them in a file only if:
- There are 100 logs if the collection
- It's been more than 1 second that no new log has been reported
- The logger is being destroyed

Here is my Log class:

public sealed class Log
{
    DateTime Date { get; }
    string Message { get; }

    public Log(string message)
        : this(DateTime.Now, message)
    { }

    private Log(DateTime writeDate, string message)
    {
        Date = writeDate;
        Message = message;
    }

    public override string ToString() => $"{Date}: {Message}";

    public static implicit operator Log(string message) => new Log(message);
}

This is the LogCollection (not an ICollection because I don't want to allow removals):

internal class LogCollection
{
    private Log[] Logs { get; }
    private int CurrentIndex { get; set; }
    private bool IsFull => CurrentIndex == Logs.Length;

    private Timer Timer { get; }

    public LogCollection() : this(100, 1000) { }

    public LogCollection(int capacity, int interval)
    {
        Logs = new Log[capacity];
        Timer = new Timer(interval);
        Timer.Elapsed += TimerIntervalElapsed;
    }

    ~LogCollection()
    {
        ReadyingLogs();
    }

    public void Add(Log log)
    {
        Logs[CurrentIndex++] = log;
        Timer.Start();

        if (IsFull)
            ReadyingLogs();
    }

    public void Clear() => CurrentIndex = 0;

    private void TimerIntervalElapsed(object sender, ElapsedEventArgs args) => ReadyingLogs();

    private void ReadyingLogs()
    {
        Timer.Stop();
        int count = CurrentIndex;
        Clear();
        LogsReady?.Invoke(this, new EventArgs(Logs.Take(count).ToArray()));
    }

    internal class EventArgs : System.EventArgs { public Log[] Logs { get; } public EventArgs(Log[] logs) { Logs = logs; } }
    internal delegate void EventHandler(object sender, EventArgs args);
    public event EventHandler LogsReady;
}

And finally the FileLogger itself:

public sealed class FileLogger // : ILogger
{
    public string FilePath { get; }
    private LogCollection Logs { get; set; }

    public FileLogger()
    {
        FilePath = Path.Combine(
            Directory.GetCurrentDirectory(),
            "Logs",
            DateTime.Today.ToString("yyyyMMdd") + ".log"
        );
        InitializeFilePath();
        InitializeCollection();
    }

    public FileLogger(string filePath)
    {
        FilePath = filePath;
        InitializeFilePath();
        InitializeCollection();
    }

    private void InitializeFilePath()
    {
        string directory = Path.GetDirectoryName(FilePath);

        if (!Directory.Exists(directory))
            Directory.CreateDirectory(directory);

        if (!File.Exists(FilePath))
            File.Create(FilePath);
    }

    private void InitializeCollection()
    {
        Logs = new LogCollection();
        Logs.LogsReady += LogsReady;
    }

    public void Log(string message) => Logs.Add(message);

    private void LogsReady(object sender, LogCollection.EventArgs args) => WriteLogs(args.Logs);
    private void WriteLogs(Log[] logs)
    {
        using (StreamWriter writer = new StreamWriter(FilePath, true))
        {
            foreach (Log log in logs)
                writer.WriteLine(log + $" (wrote at {DateTime.Now.ToString("hh:mm:ss:fff")})");
            writer.Close();
        }
    }
}

It works as intended for a single thread with this ConsoleApplication:

class Program
{
    static void Main(string[] args)
    {
        FileLogger logger = new FileLogger();

        if (File.Exists(logger.FilePath))
            File.Delete(logger.FilePath);

        for (int i = 0; i < 150; i++)
            logger.Log($"This is log #{i + 1,0:000}");

        Thread.Sleep(1500);

        for (int i = 0; i < 50; i++)
            logger.Log($"This is log #{i + 151,0:000}");
    }
}

Output:

The first hundred of logs are wrote at t=0ms all together (pack of 100)
The fifty next logs are wrote at t=1050ms all together (timer elapsed)
The fifty last logs are wrote at t=1500ms all together (logger destroyed)


This is for a single thread, but I am not comfortable with multi-threading. Is this safe if I use it in this state with multiple threads? I want to keep the chronology, and of course do not loose any log...

\$\endgroup\$
4
\$\begingroup\$

This design does not convince me. You would achieve much better manintainability and testability when the buffering functionality would be implemented as a decorator. This is, create another logger that takes some other logger as a dependency and adds buffering on top of it. You would then be able to test it without having to actually log anything. You would also be able to reuse it for other targets if you later decide to log to a database just use the decorator with it. Currently there is no per logger choice. Either all loggers are buffered or none of them and all of them must use the same settings. With a decorator you could specify it for each logger separately or not at all.

The LoggerCollection should not be doing the buffering. As the name suggests it's a collection so it just should store loggers.


This is the LogCollection (not an ICollection because I don't want to allow removals):

Understandable but not entirely correct. ICollection has a property for that, that is called IsReadOnly so you could return true here and disallow changing it. Another option would be to implement just the IEnumerable<T> interface to make it a real collection. It's very misleading to have a class named SomethingCollection but not providing any of the expected collection interfaces.


internal class EventArgs : System.EventArgs { public Log[] Logs { get; } public EventArgs(Log[] logs) { Logs = logs; } }

ok, I have to add that this is really the most unusual and crazy thing to do, define a class in a single line. This is super unacceptable.

I also don't like the entire idea with the event loop. Events are fine but like here they sometimes add unnecessary complexity and hidden dependencies.

btw, if you create your own EventArgs then name it appropriately, don't just use the same name as the base class. This is another unacceptable style. You should then also use the EventHandler<T> class with the generic argument and not the simple one.

\$\endgroup\$
  • 1
    \$\begingroup\$ @Heslacher thanks! I need to pay more attention to the correct spelling ;-) \$\endgroup\$ – t3chb0t Nov 5 '18 at 7:29
1
\$\begingroup\$

To answer your question if it's thread safe. It is not. When dealing with threads you need to think that the same piece of code could be hit at anytime. For example two threads could hit the IsFull and both try to submit the logger. Also one thread could have hit stop on the timer and another one before the next line runs start the timer. Now you could have more logs than your array holds. You will need to change your code to do locks or I would suggest look at the producer consumer pattern.

The TPL DataFlow blocks are great for this pattern. It's the System.Threading.Tasks.Dataflow nugget package. Here's an example using the TPL DataFlow Blocks. I didn't do a finalizer as I think IDisposable is the pattern you should use. There is no Clear for the Data Mesh either.

public class Logger : IDisposable
{
    private string _filePath;
    private BufferBlock<Log> _queueBlock;
    private bool _isDisposed;

    public Logger(int capacity, TimeSpan interval, string filePath)
    {
        if (capacity < 1)
        {
            throw new ArgumentOutOfRangeException(nameof(capacity));
        }

        InitializeFilePath(filePath);
        InitializeDataMesh(capacity, interval);
    }

    private void InitializeDataMesh(int capacity, TimeSpan interval)
    {
        // This will do your grouping
        var batchBlock = new BatchBlock<Log>(capacity);

        // This timer will handle the sliding time window
        var timer = new Timer(_ => batchBlock.TriggerBatch());

        // Block to trigger reset on timer if new value added
        // see https://stackoverflow.com/a/9423830
        var resetBlock = new TransformBlock<Log, Log>(value =>
        {
            timer.Change(interval, Timeout.InfiniteTimeSpan);
            return value;
        });

        var writerBlock = new ActionBlock<Log[]>(logs => WriteLogs(logs), new ExecutionDataflowBlockOptions()
        {
            BoundedCapacity = 1, // only want to have WriteLogs run one at a time
        });

        // Queue to hold the adds
        _queueBlock = new BufferBlock<Log>();

        batchBlock.LinkTo(writerBlock, new DataflowLinkOptions()
        {
            PropagateCompletion = true
        });

        resetBlock.LinkTo(batchBlock, new DataflowLinkOptions()
        {
            PropagateCompletion = true
        });

        _queueBlock.LinkTo(resetBlock, new DataflowLinkOptions()
        {
            PropagateCompletion = true
        });
    }

    private void InitializeFilePath(string filePath)
    {
        if (string.IsNullOrWhiteSpace(filePath))
        {
            filePath = Path.Combine(
                Directory.GetCurrentDirectory(),
                "Logs",
                DateTime.Today.ToString("yyyyMMdd") + ".log"
            );
        }

        string directory = Path.GetDirectoryName(filePath);

        if (!Directory.Exists(directory))
        {
            Directory.CreateDirectory(directory);
        }

        _filePath = filePath;
    }

    private void WriteLogs(Log[] logs)
    {
        using (StreamWriter writer = new StreamWriter(_filePath, true))
        {
            var logDate = DateTime.Now; // Moved log date out of loop
            foreach (Log log in logs)
            {
                writer.WriteLine(log + $" (wrote at {logDate:hh:mm:ss:fff})");
            }
            writer.Close();
        }
    }

    public void Add(Log log)
    {
        if (_isDisposed)
        {
            throw new ObjectDisposedException(typeof(Logger).FullName);
        }
        _queueBlock.Post(log);
    }

    public Task AddAsync(Log log, CancellationToken token)
    {
        if (_isDisposed)
        {
            throw new ObjectDisposedException(typeof(Logger).FullName);
        }

        return _queueBlock.SendAsync(log, token);
    }

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

    protected virtual void Dispose(bool disposing)
    {
        if (_isDisposed)
        {
            return;
        }

        if (disposing)
        {
            _queueBlock.Complete();
        }

        _isDisposed = true;
    }
}

There is no LogCollection now as the DataFlow blocks handle that.

To test in your ConsoleApp

static void Main(string[] args)
{

    using (var logger = new Logger(100, TimeSpan.FromSeconds(1), string.Empty))
    {
        Parallel.ForEach(Enumerable.Range(0, 149), i => logger.Add($"This is log #{i + 1,0:000}"));

        Thread.Sleep(1500);

        Parallel.ForEach(Enumerable.Range(0, 49), i => logger.Add($"This is log #{i + 151,0:000}"));

        Thread.Sleep(1500);

        logger.Add($"This is log #{999,0:000}");
    }

    Console.Write("Done");


    Console.ReadLine();
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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