2
\$\begingroup\$

Here is a sample code to explain the approach I am going for.

public class LogWriter
{
    #region Private Members

    // If writer is not static class, still need to keep single message list; same for other members
    private static ConcurrentQueue<string> _logMessages = new ConcurrentQueue<string>();
    private static object locker = new object();
    private static bool _stopAfterCurrentQueue = false;
    private static bool _discardQueueAndStop = false;

    private static CancellationTokenSource _tokenSource = new CancellationTokenSource();
    #endregion

    public void Write(string text)
    {
        if (!_stopAfterCurrentQueue && !_discardQueueAndStop)
        {
            Task.Run(() =>
           {
               _logMessages.Enqueue(text);

           });

            Task.Run(() =>
            {
                while (!_logMessages.IsEmpty)
                {
                    foreach (var item in _logMessages)
                    {

                        _logMessages.TryDequeue(out string current);
                        lock (locker)
                        {
                            // Will be replaced by StreamWriter
                            File.AppendAllText("Log_" + DateTime.Now.ToString("yyyyMMMdd") + ".txt", current + Environment.NewLine);
                        }
                    }
                }
            }, _tokenSource.Token);
        }
    }

    public void ProcessCurrentAndStop()
    {
        // Only stops accepting new messages, will process the current queue
        _stopAfterCurrentQueue = true;
    }

    public void DiscardQueueAndStop()
    {
        // Cancels subsequent Enqueue
        _tokenSource.Cancel();

        // No more writing even if there is something in the queue
        _discardQueueAndStop = true;
    }

    public void RestartLogging()
    {
        _stopAfterCurrentQueue = false;
        _discardQueueAndStop = false;

        _tokenSource.Dispose();
        _tokenSource = new CancellationTokenSource();
    }
}

The idea is to write to file in asynchronous way. I am trying to make it thread safe as well. There is not much to process here except to write message to a file.

I would like to understand what could be potential issues with this approach and also if it would be better to keep class static.

\$\endgroup\$
  • 1
    \$\begingroup\$ You are not checking the cancellation token. \$\endgroup\$ – paparazzo May 25 '18 at 11:41
2
\$\begingroup\$

Let’s try to get rid of ConcurrentQueue. And Task.Run – those are expensive ones. It also makes sense to use async file access – which is a way more lightweight.

public class LogWriter
{
    public static LogWriter Instance = new LogWriter();

    LogWriter()
    {
        Enabled = true;
        Cts = new CancellationTokenSource();
        Task = Task.CompletedTask;
    }

    bool Enabled { get; set; }
    CancellationTokenSource Cts { get; set; } 
    Task Task { get; set; }
    string Path => $"Log_{DateTime.Now:yyyyMMMdd}.txt";

    public void Start() => Enabled = true;

    public void Stop(bool discard = false)
    {
        Enabled = false;
        if (discard)
        {
            Cts.Cancel();
            Cts = new CancellationTokenSource();
            Task = Task.ContinueWith(t => { });
        }
    }

    public void Write(string line) =>
        Write(line, Path, Cts.Token);

    void Write(string line, string path, CancellationToken token)
    {
        if (Enabled)
            lock(Task)
                Task = Task.ContinueWithAsync(
                    t => AppendAllTextAsync(path, line + NewLine, token),
                    token);
    }        
}

Where missing part would be:

static class AsyncContinuations
{
    public static async Task ContinueWithAsync(this Task task, Func<Task, Task> continuationFunction, CancellationToken cancellationToken)
    {
        await task;
        cancellationToken.ThrowIfCancellationRequested();
        await continuationFunction(task);
    }
}
\$\endgroup\$
  • \$\begingroup\$ @t3chb0t – how do you like this scheduling mechanism. I could not believe it – Microsoft TPL ContinueWith() does not support async actions! And C# does not allow a sleek overload for that… \$\endgroup\$ – Dmitry Nogin Jun 26 '18 at 4:29
  • \$\begingroup\$ I first have to reformat it to something that my brain can deal with :-P There are too many missing {} and since I really dislike python I have to re-add them ;-) I'm also wondering that I didn't get any notification from SE, I found your comment by accident :-o \$\endgroup\$ – t3chb0t Jun 26 '18 at 8:02
  • \$\begingroup\$ @t3chb0t Have you ever witnessed a grammar reform in a human language? Yep, that one is a real pain to the eyes :) Have a look at F# - around 4 times less noise, you could quickly become addicted. Old C# style is so dramatically outdated.... It is just like a modern day Pascal :) \$\endgroup\$ – Dmitry Nogin Jun 27 '18 at 2:42
0
\$\begingroup\$

I think I would build up the append and then just take the Lock once. Write all at once is going to be more efficient.

public static void WriteTask(string text)
{
    if (!_stopAfterCurrentQueue && !_discardQueueAndStop  && !string.IsNullOrEmpty(text))
    {
        _logMessages.Enqueue(text);

        List<string> lString = new List<string>();
        string current;
        while (_logMessages.TryDequeue(out current))
        {
            lString.Add(current);
        }

        lock (locker)
        {
            File.AppendAllLines("Log_" + DateTime.Now.ToString("yyyyMMMdd") + ".txt", lString);
        }
    }
}
\$\endgroup\$
0
\$\begingroup\$

I think Parallel.Invoke() is better than Task.Run(). Parallel.Invoke() is best performance wise for parallel task also interface object is not Disposed after Completed HTTP request. it's working as background task.

public class LogWriter
{
    #region Private Members

    // If writer is not static class, still need to keep single message list; same for other members
    private static ConcurrentQueue<string> _logMessages = new ConcurrentQueue<string>();
    private static object locker = new object();
    private static bool _stopAfterCurrentQueue = false;
    private static bool _discardQueueAndStop = false;

    private static CancellationTokenSource _tokenSource = new CancellationTokenSource();
    private static readonly SemaphoreSlim _messageEnqueuedSignal = new SemaphoreSlim(0);
    #endregion

    public static void Write(string text)
    {

        if (!_tokenSource.IsCancellationRequested)
        {
            if (!_stopAfterCurrentQueue && !_discardQueueAndStop)
            {
                Parallel.Invoke(() =>
                {
                    _logMessages.Enqueue(text);
                    _messageEnqueuedSignal.Release();
                });

                Parallel.Invoke( async () =>
                    {
                        await _messageEnqueuedSignal.WaitAsync(_tokenSource.Token);
                        while (!_logMessages.IsEmpty)
                        {
                            foreach (var item in _logMessages)
                            {

                                _logMessages.TryDequeue(out string current);
                                lock (locker)
                                {
                                // Will be replaced by StreamWriter
                                File.AppendAllText("Log_" + DateTime.Now.ToString("yyyyMMMdd") + ".txt", current + Environment.NewLine);
                                }
                            }
                        }
                    });
            }
        }
    }

    public void ProcessCurrentAndStop()
    {
        // Only stops accepting new messages, will process the current queue
        _stopAfterCurrentQueue = true;
    }

    public void DiscardQueueAndStop()
    {
        // Cancels subsequent Enqueue
        _tokenSource.Cancel();

        // No more writing even if there is something in the queue
        _discardQueueAndStop = true;
    }

    public void RestartLogging()
    {
        _stopAfterCurrentQueue = false;
        _discardQueueAndStop = false;

        _tokenSource.Dispose();
        _tokenSource = new CancellationTokenSource();
    }
}
\$\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.