0

I was creating a small logger. For that purpose I have a class (LogFile) which publishes the Log(ILogCsvLine csvLine) method. This method adds the line to log to a queue (linesToLog) and sets a trigger, that has been registered to the ThreadPool with Logging(..) as method that should be executed on a different thread whenever there is a trigger and pending processor time. The Logging(..) method is writing the lines to log to a given file. Now I ran into the problem, that the Dispose() method has been called, while the queue wasn't empty, resulting in a call to the Logging(..) method, while trigger or fileAccessLock were already disposed. As a solution I've build some checks around those EventWaitHandles and was wondering whether there is a better readable and more elegant way of doing this.

 internal sealed class LogFile : ILogFile
{
    private readonly EventWaitHandle fileAccessLock = new AutoResetEvent(true);
    private readonly IFilesLoader filesLoader;
    private readonly Queue<ILogCsvLine> linesToLog = new Queue<ILogCsvLine>();
    private readonly IFile logFile;
    private readonly object myLock = new object();
    private RegisteredWaitHandle registeredWait;
    private readonly EventWaitHandle trigger = new AutoResetEvent(false);

    private IDirectory directory = null;
    private bool disposeFileAccessLock = false;
    private bool disposeTrigger = false;
    private bool isDisposed = false;
    private bool isDisposing = false;
    private bool isLogging = false;
    private bool isSettingTrigger = false;

    public event EventHandler<FilesException> LoggingFailed;
        
    public LogFile(IFile logFile, IFilesLoader filesLoader)
    {
        this.filesLoader = filesLoader;
        this.logFile = logFile;
        Setup();
    }

    private void Setup()
    {
        directory = logFile.ParentDirectory;
        EnforceFileExists();
        registeredWait = ThreadPool.RegisterWaitForSingleObject(trigger, Logging, null, -1, false);
    }

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

    ~LogFile()
    {
        Dispose(false);
    }

    private void Dispose(bool disposing)
    {
        if (!isDisposed)
        {
            try
            {
                lock (myLock)
                    isDisposing = true;
                registeredWait?.Unregister(trigger);
                if (disposing)
                {
                    if (isSettingTrigger)
                        disposeTrigger = true;
                    else
                        trigger?.Dispose();
                    if (isLogging)
                        disposeFileAccessLock = true;
                    else
                        fileAccessLock?.Dispose();
                }
            }
            finally
            {
                isDisposed = true;
                lock (myLock)
                    isDisposing = false;
            }
        }
    }

    public IFile File => logFile;

    public void Log(ILogCsvLine csvLine)
    {
        lock (myLock)
        {
            if (isDisposing || isDisposed)
                return;
            linesToLog.Enqueue(csvLine);
            isSettingTrigger = true;
        }
        trigger.Set();
        lock (myLock)
        {
            isSettingTrigger = false;
            if (disposeTrigger)
                trigger?.Dispose();
        }
    }

    private void Logging(object data, bool timedOut)
    {
        ILogCsvLine line = null;
        lock (myLock)
        {
            if (linesToLog.Count == 0)
                return;
            if (isDisposing || isDisposed)
                return;
            line = linesToLog.Dequeue();
            isLogging = true;
        }
        fileAccessLock.WaitOne();
        FilesException occurredException = null;
        IStreamWriter sw = null;
        try
        {
            EnforceFileExists();
            sw = logFile.AppendText();
            do
            {
                sw.WriteLine(line.ToCsvString());
                lock (myLock)
                {
                    if (linesToLog.Count > 0)
                        line = linesToLog.Dequeue();
                    else
                        line = null;
                }
            } while (line != null);
        }
        catch (Exception e)
        {
            if (e is ThreadAbortException)
                throw;
            string message = string.Format("Error writing to {0}. {1}", logFile.Path, e.Message);
            occurredException = new FilesException(message, e);
        }
        finally
        {
            if (sw != null)
            {
                sw.Flush();
                sw.Close();
                sw = null;
            }
        }
        fileSizeManager?.Check();
        fileAccessLock.Set();
        lock (myLock)
        {
            if (disposeFileAccessLock)
                fileAccessLock?.Dispose();
            isLogging = false;
        }
        if (occurredException != null)
            LoggingFailed?.Invoke(this, occurredException);
    }

    private void EnforceFileExists()
    {
        if (!directory.Exists)
            directory.Create();
        if (!logFile.Exists)
        {
            var fileAccess = filesLoader.GetFileAccess();
            fileAccess.Create(logFile.Path, FileSystemRights.Read | FileSystemRights.Write);
        }
    }
}
Der
  • 3
  • 3
  • There is a lot going on here and some of it seems a little overbaked.. Why not Just use a logging framework like Serilog, or one of the older guys?. I mean they have already solved these problems and have lots of support you will take a long time to get right – TheGeneral Jun 25 '20 at 08:37
  • Why do you have finalizer? – cassandrad Jun 25 '20 at 08:38
  • @TheGeneral I know there are logging frameworks, but I thought doing it myself is a good way of theaching me how stuff is done. – Der Jun 25 '20 at 10:03
  • @cassandrad: I used the finalizer because I thought that it is required for the Dispose pattern. – Der Jun 25 '20 at 10:04

0 Answers0