Writing to file using TPL with ConcurrentQueue
up vote
1
down vote
favorite
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.
c# task-parallel-library
asked May 25 at 4:26
danish
1372
add a comment |
up vote
1
down vote
favorite
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.
c# task-parallel-library
asked May 25 at 4:26
danish
1372
1
You are not checking the cancellation token.
– paparazzo
May 25 at 11:41
add a comment |
up vote
1
down vote
favorite
up vote
1
down vote
favorite
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.
c# task-parallel-library
asked May 25 at 4:26
danish
1372
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.
c# task-parallel-library
c# task-parallel-library
asked May 25 at 4:26
danish
1372
asked May 25 at 4:26
danish
1372
asked May 25 at 4:26
danish
1372
asked May 25 at 4:26
danish
1372
asked May 25 at 4:26
danish
1372
1372
1
You are not checking the cancellation token.
– paparazzo
May 25 at 11:41
add a comment |
1
You are not checking the cancellation token.
– paparazzo
May 25 at 11:41
1
1
You are not checking the cancellation token.
– paparazzo
May 25 at 11:41
You are not checking the cancellation token.
– paparazzo
May 25 at 11:41
add a comment |
4 Answers
4
active
oldest
votes
up vote
1
down vote
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);
}
}
answered Jun 26 at 4:28
Dmitry Nogin
2,901625
@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…
– Dmitry Nogin
Jun 26 at 4:29
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
– t3chb0t
Jun 26 at 8:02
@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 :)
– Dmitry Nogin
Jun 27 at 2:42
add a comment |
up vote
0
down vote
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);
}
}
}
answered May 26 at 18:12
paparazzo
4,9911733
add a comment |
up vote
0
down vote
The BlockingCollection<T> is a much better tool for implementing the the Producer-Consumer pattern.
A really simple implementaion could look like this where you create a Task that is a LongRunning one that consumes the collection and a Write method that enqueues new items. The Completion is made public on purpose so that you can wait until everything is processed just before you exit your application .
public class Logger
{
private static BlockingCollection<string> _logs = new BlockingCollection<string>();
public static Task Completion { get; } = Task.Factory.StartNew(() =>
{
foreach (var log in _logs.GetConsumingEnumerable())
{
Console.WriteLine(log);
}
}, TaskCreationOptions.LongRunning);
public static void Write(string text)
{
_logs.Add(text);
}
}
Alternatively you can use the Dataflow (Task Parallel Library) and just build it of ready-made components. They offer a lot of options too so check the documentation.
public class Logger2
{
private static BufferBlock<string> _logs;
private static ActionBlock<string> _logger;
static Logger2()
{
_logs = new BufferBlock<string>();
_logger = new ActionBlock<string>(l => Log(l));
_logs.LinkTo(_logger);
}
public static Task Completion => _logs.Completion;
public static void Write(string text)
{
_logs.Post(text);
}
private static void Log(string log) => Console.WriteLine(log);
}
answered Jul 6 at 17:32
t3chb0t
33.7k744108
add a comment |
up vote
0
down vote
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();
}
}
edited 2 days ago
Sᴀᴍ Onᴇᴌᴀ
7,74061748
answered 2 days ago
Khushali
11
New contributor
Khushali is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
add a comment |
4 Answers
4
active
oldest
votes
4 Answers
4
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
1
down vote
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);
}
}
answered Jun 26 at 4:28
Dmitry Nogin
2,901625
@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…
– Dmitry Nogin
Jun 26 at 4:29
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
– t3chb0t
Jun 26 at 8:02
@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 :)
– Dmitry Nogin
Jun 27 at 2:42
add a comment |
up vote
1
down vote
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);
}
}
answered Jun 26 at 4:28
Dmitry Nogin
2,901625
@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…
– Dmitry Nogin
Jun 26 at 4:29
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
– t3chb0t
Jun 26 at 8:02
@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 :)
– Dmitry Nogin
Jun 27 at 2:42
add a comment |
up vote
1
down vote
up vote
1
down vote
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);
}
}
answered Jun 26 at 4:28
Dmitry Nogin
2,901625
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);
}
}
answered Jun 26 at 4:28
Dmitry Nogin
2,901625
edited Jun 26 at 4:38
answered Jun 26 at 4:28
Dmitry Nogin
2,901625
answered Jun 26 at 4:28
Dmitry Nogin
2,901625
answered Jun 26 at 4:28
Dmitry Nogin
2,901625
2,901625
@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…
– Dmitry Nogin
Jun 26 at 4:29
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
– t3chb0t
Jun 26 at 8:02
@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 :)
– Dmitry Nogin
Jun 27 at 2:42
add a comment |
@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…
– Dmitry Nogin
Jun 26 at 4:29
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
– t3chb0t
Jun 26 at 8:02
@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 :)
– Dmitry Nogin
Jun 27 at 2:42
@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…
– Dmitry Nogin
Jun 26 at 4:29
@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…
– Dmitry Nogin
Jun 26 at 4:29
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– t3chb0t
Jun 26 at 8:02
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– t3chb0t
Jun 26 at 8:02
@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 :)
– Dmitry Nogin
Jun 27 at 2:42
@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 :)
– Dmitry Nogin
Jun 27 at 2:42
add a comment |
up vote
0
down vote
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);
}
}
}
answered May 26 at 18:12
paparazzo
4,9911733
add a comment |
up vote
0
down vote
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);
}
}
}
answered May 26 at 18:12
paparazzo
4,9911733
add a comment |
up vote
0
down vote
up vote
0
down vote
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);
}
}
}
answered May 26 at 18:12
paparazzo
4,9911733
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);
}
}
}
answered May 26 at 18:12
paparazzo
4,9911733
answered May 26 at 18:12
paparazzo
4,9911733
answered May 26 at 18:12
paparazzo
4,9911733
answered May 26 at 18:12
paparazzo
4,9911733
4,9911733
add a comment |
add a comment |
up vote
0
down vote
The BlockingCollection<T> is a much better tool for implementing the the Producer-Consumer pattern.
A really simple implementaion could look like this where you create a Task that is a LongRunning one that consumes the collection and a Write method that enqueues new items. The Completion is made public on purpose so that you can wait until everything is processed just before you exit your application .
public class Logger
{
private static BlockingCollection<string> _logs = new BlockingCollection<string>();
public static Task Completion { get; } = Task.Factory.StartNew(() =>
{
foreach (var log in _logs.GetConsumingEnumerable())
{
Console.WriteLine(log);
}
}, TaskCreationOptions.LongRunning);
public static void Write(string text)
{
_logs.Add(text);
}
}
Alternatively you can use the Dataflow (Task Parallel Library) and just build it of ready-made components. They offer a lot of options too so check the documentation.
public class Logger2
{
private static BufferBlock<string> _logs;
private static ActionBlock<string> _logger;
static Logger2()
{
_logs = new BufferBlock<string>();
_logger = new ActionBlock<string>(l => Log(l));
_logs.LinkTo(_logger);
}
public static Task Completion => _logs.Completion;
public static void Write(string text)
{
_logs.Post(text);
}
private static void Log(string log) => Console.WriteLine(log);
}
answered Jul 6 at 17:32
t3chb0t
33.7k744108
add a comment |
up vote
0
down vote
The BlockingCollection<T> is a much better tool for implementing the the Producer-Consumer pattern.
A really simple implementaion could look like this where you create a Task that is a LongRunning one that consumes the collection and a Write method that enqueues new items. The Completion is made public on purpose so that you can wait until everything is processed just before you exit your application .
public class Logger
{
private static BlockingCollection<string> _logs = new BlockingCollection<string>();
public static Task Completion { get; } = Task.Factory.StartNew(() =>
{
foreach (var log in _logs.GetConsumingEnumerable())
{
Console.WriteLine(log);
}
}, TaskCreationOptions.LongRunning);
public static void Write(string text)
{
_logs.Add(text);
}
}
Alternatively you can use the Dataflow (Task Parallel Library) and just build it of ready-made components. They offer a lot of options too so check the documentation.
public class Logger2
{
private static BufferBlock<string> _logs;
private static ActionBlock<string> _logger;
static Logger2()
{
_logs = new BufferBlock<string>();
_logger = new ActionBlock<string>(l => Log(l));
_logs.LinkTo(_logger);
}
public static Task Completion => _logs.Completion;
public static void Write(string text)
{
_logs.Post(text);
}
private static void Log(string log) => Console.WriteLine(log);
}
answered Jul 6 at 17:32
t3chb0t
33.7k744108
add a comment |
up vote
0
down vote
up vote
0
down vote
The BlockingCollection<T> is a much better tool for implementing the the Producer-Consumer pattern.
A really simple implementaion could look like this where you create a Task that is a LongRunning one that consumes the collection and a Write method that enqueues new items. The Completion is made public on purpose so that you can wait until everything is processed just before you exit your application .
public class Logger
{
private static BlockingCollection<string> _logs = new BlockingCollection<string>();
public static Task Completion { get; } = Task.Factory.StartNew(() =>
{
foreach (var log in _logs.GetConsumingEnumerable())
{
Console.WriteLine(log);
}
}, TaskCreationOptions.LongRunning);
public static void Write(string text)
{
_logs.Add(text);
}
}
Alternatively you can use the Dataflow (Task Parallel Library) and just build it of ready-made components. They offer a lot of options too so check the documentation.
public class Logger2
{
private static BufferBlock<string> _logs;
private static ActionBlock<string> _logger;
static Logger2()
{
_logs = new BufferBlock<string>();
_logger = new ActionBlock<string>(l => Log(l));
_logs.LinkTo(_logger);
}
public static Task Completion => _logs.Completion;
public static void Write(string text)
{
_logs.Post(text);
}
private static void Log(string log) => Console.WriteLine(log);
}
answered Jul 6 at 17:32
t3chb0t
33.7k744108
The BlockingCollection<T> is a much better tool for implementing the the Producer-Consumer pattern.
A really simple implementaion could look like this where you create a Task that is a LongRunning one that consumes the collection and a Write method that enqueues new items. The Completion is made public on purpose so that you can wait until everything is processed just before you exit your application .
public class Logger
{
private static BlockingCollection<string> _logs = new BlockingCollection<string>();
public static Task Completion { get; } = Task.Factory.StartNew(() =>
{
foreach (var log in _logs.GetConsumingEnumerable())
{
Console.WriteLine(log);
}
}, TaskCreationOptions.LongRunning);
public static void Write(string text)
{
_logs.Add(text);
}
}
Alternatively you can use the Dataflow (Task Parallel Library) and just build it of ready-made components. They offer a lot of options too so check the documentation.
public class Logger2
{
private static BufferBlock<string> _logs;
private static ActionBlock<string> _logger;
static Logger2()
{
_logs = new BufferBlock<string>();
_logger = new ActionBlock<string>(l => Log(l));
_logs.LinkTo(_logger);
}
public static Task Completion => _logs.Completion;
public static void Write(string text)
{
_logs.Post(text);
}
private static void Log(string log) => Console.WriteLine(log);
}
answered Jul 6 at 17:32
t3chb0t
33.7k744108
answered Jul 6 at 17:32
t3chb0t
33.7k744108
answered Jul 6 at 17:32
t3chb0t
33.7k744108
answered Jul 6 at 17:32
t3chb0t
33.7k744108
33.7k744108
add a comment |
add a comment |
up vote
0
down vote
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();
}
}
edited 2 days ago
Sᴀᴍ Onᴇᴌᴀ
7,74061748
answered 2 days ago
Khushali
11
New contributor
Khushali is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
add a comment |
up vote
0
down vote
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();
}
}
edited 2 days ago
Sᴀᴍ Onᴇᴌᴀ
7,74061748
answered 2 days ago
Khushali
11
New contributor
Khushali is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
add a comment |
up vote
0
down vote
up vote
0
down vote
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();
}
}
edited 2 days ago
Sᴀᴍ Onᴇᴌᴀ
7,74061748
answered 2 days ago
Khushali
11
New contributor
Khushali is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
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();
}
}
edited 2 days ago
Sᴀᴍ Onᴇᴌᴀ
7,74061748
answered 2 days ago
Khushali
11
New contributor
Khushali is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
edited 2 days ago
Sᴀᴍ Onᴇᴌᴀ
7,74061748
edited 2 days ago
Sᴀᴍ Onᴇᴌᴀ
7,74061748
edited 2 days ago
Sᴀᴍ Onᴇᴌᴀ
7,74061748
7,74061748
answered 2 days ago
Khushali
11
New contributor
Khushali is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
answered 2 days ago
Khushali
11
answered 2 days ago
Khushali
11
11
New contributor
Khushali is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
New contributor
Khushali is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Khushali is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
add a comment |
add a comment |
draft saved
draft discarded
draft saved
draft discarded
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f195131%2fwriting-to-file-using-tpl-with-concurrentqueue%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
1
You are not checking the cancellation token.
– paparazzo
May 25 at 11:41