|
Can I ask you your opinion if you see some major problems with this class I wrote? As you can see this is meant to be thread safe log class.
I don't want to use 3rd party solution. Thanks.
static class GMLogger
{
private static List<string> _buffer = new List<string>();
private static int _maxBufferSize = 25;
public static string _logDir = "C:\\Logs";
private static readonly object _syncObject = new object();
public static void Log(string logMessage)
{
try
{
lock (_syncObject)
{
_buffer.Add(logMessage);
Save(_buffer);
}
}
catch (Exception ex)
{
throw;
}
}
private static void Save(List<string> buffer)
{
if (buffer.Count > _maxBufferSize)
{
if (!_logDir.EndsWith("\\")) _logDir += "\\";
DirectoryInfo di = Directory.CreateDirectory(_logDir);
var todaysLogFilePath = Path.Combine(_logDir,
("Log" + DateTime.Now.ToString("yyyy-MMMM-dd") + ".txt"));
using (FileStream aFile = new FileStream(todaysLogFilePath, FileMode.Append, FileAccess.Write, FileShare.None))
using (StreamWriter sw = new StreamWriter(aFile))
{
for (int i = 0; i < buffer.Count(); i++)
{
sw.WriteLine(buffer[i]);
}
}
buffer.Clear();
}
}
}
|
|
|
|
|
I'm in the habit of searching CodeProject for articles that demonstrate the functionality I look for: in this case, I'd sure want to read: "Simple Log" by Jochen Scharr [^] "Essential logging in one simple, robust, static, thread-safe class. No initialization, no configuration, no dependencies."
What "jumps out" at me from the code is your use of '_buffer and 'buffer: since you are not passing '_buffer by reference to the 'Save method, is '_buffer ever really getting cleared when the 'Save method calls 'buffer.Clear(); ?
On the "big picture" level:
1. I would most probably want to create a "daily log" class which I'd serialize to a stream, and then append to a "master" log file.
2. I'd evaluate if there's anywhere I could use a StringBuilder here.
3. I'd test for the save Directory's existence, perhaps set a boolean flag to indicate it's defined. I'd create the day's log file once, and check whether it's up to date.
«I want to stay as close to the edge as I can without going over. Out on the edge you see all kinds of things you can't see from the center» Kurt Vonnegut.
|
|
|
|
|
about buffer that's a typo thanks. Apart all people usually day don't write your own, but I don't understand what can go wrong with this class?
In that way also the article you linked, maybe that one also isn't that reliable?
|
|
|
|
|
user20044 wrote: maybe that one also isn't that reliable
Hi, I'm not sure; I haven't used it: I wrote my own logger; it uses a collection of 'Log Class "instances" which is written to a stream, compressed with GZip, and, then, serialized. But, it's not written to be thread-safe, something I intend to get around to doing when it's called for.
In terms of "what can go wrong" with your implementation, well: you need to test it.
cheers, Bill
«I want to stay as close to the edge as I can without going over. Out on the edge you see all kinds of things you can't see from the center» Kurt Vonnegut.
|
|
|
|
|
Hmm tested and it works. Your link seems a nice class I would really appreciate if you or someone else can tell me if that class is reliable?
|
|
|
|
|
Hopefully, you'll get some other responses here that are useful to you.
«I want to stay as close to the edge as I can without going over. Out on the edge you see all kinds of things you can't see from the center» Kurt Vonnegut.
|
|
|
|
|
Actually your comment about buffer! - initially it was written correctly and it would still clear the original member variable _buffer, since buffer points to _buffer
|
|
|
|
|
The first thing that jumps out at me:
try
{
...
}
catch (Exception ex)
{
throw;
}
You've avoided the common mistake of using throw ex;, which would destroy the call-stack; but you're not doing anything with the exception that you've caught. You should remove the entire try..catch and replace it with the contents of the try block.
You're calling buffer.Count(), which is an extension method provided by LINQ. Although it has special-case code for collections which implement ICollection<T>, it would be cleaner to just use the Count property directly on the list.
Use string.Format to build the file name would make the code easier to read:
var todaysLogFilePath = Path.Combine(_logDir, string.Format("Log{0:yyyy-MMMM-dd}.txt", DateTime.Today));
Alternatively, if you're using .NET 4.6, an interpolated string would be even cleaner:
var todaysLogFilePath = Path.Combine(_logDir, $"Log{DateTime.Today:yyyy-MMMM-dd}.txt");
I'd be inclined to replace the whole FileStream / StreamWriter block with a single call to File.AppendAllLines:
var todaysLogFilePath = ...;
File.AppendAllLines(todaysLogFilePath, buffer);
buffer.Clear();
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Thanks for feedback I am using .NET 4.
I used file stream because of FileShare.None property to handle case if different processes using my class want to write to same file.
My biggest concern now is that people start to complain when they see my class that I should not design logging class myself. But I want to have just a simple class. No config files etc.
And now I am not sure if it is reliable to use my class? From different threads? From different processes? Can someone comment on that?
|
|
|
|
|
FileShare.Read should be sufficient to prevent multiple processes from writing to the file at the same time, and that's the mode that File.AppendAllLines uses.
Within a single AppDomain, your class should be perfectly thread-safe - everything is wrapped in a monitor on the same guard object, so only one thread can be executing the code at a time.
With different AppDomains, or different processes, things get more complicated. Your code can be executing on threads from different AppDomains or processes at the same time, because they have their own copies of the buffer and the guard object. The problem you will encounter is that the FileStream constructor (or the AppendAllLines method) will throw an exception if another process is writing to the file when you try to open it.
There's no easy way around that last problem, which is why most logging frameworks don't tend to write to the same file from different processes.
If you need multiple processes logging to the same place at the same time, you might want to consider the Event Log, and possibly look at using the Microsoft.Diagnostics.Tracing.EventSource library[^] (NuGet[^] | MSDN[^]) to simplify your code.
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
With different AppDomains, or different processes, things get more complicated. Your code can be executing on threads from different AppDomains or processes at the same time, because they have their own copies of the buffer and the guard object. The problem you will encounter is that the FileStream constructor (or the AppendAllLines method) will throw an exception if another process is writing to the file when you try to open it.
This is fine. Let it throw. However when one process is done with it, I hope other process can now write.
Thanks for your explanation you are helping me unlike some comments I get like "don't create log classes" - please see discussion below the answer here: http://codereview.stackexchange.com/questions/108280/implementing-a-thread-safe-log-class-with-simple-functionality/108286?noredirect=1[^]
So what is your opinion do you think it is worth to use my class in my application?
|
|
|
|
|
user20044 wrote: So what is your opinion do you think it is worth to use my class in my application?
If it does what you need, then I don't see any reason not to use it. 
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Well functionally it does but since people complain not design your log class I am getting confused 
Nikitas first point is nothing surely I can make that field private.
Third point I can set buffer size to one actually and since I am only logging ERRORS I can expect there will not be much entries going to be written in the log, doesn't it make sense?
|
|
|
|
|
user20044 wrote: I can set buffer size to one
In that case, you can significantly simplify the class:
static class GMLogger
{
private const string LogDirectory = @"C:\Logs\";
private static readonly object _syncObject = new object();
private static string EnsureLogDirectory()
{
string path = LogDirectory;
Directory.CreateDirectory(path);
return path;
}
private static string BuildLogPath(string directory, DateTime logDate)
{
string fileName = string.Format("Log{0:yyyy-MM-dd}.txt", logDate);
return Path.Combine(directory, fileName);
}
public static void Log(string logMessage)
{
lock (_syncObject)
{
string directory = EnsureLogDirectory();
string fileName = BuildLogPath(directory, DateTime.Today);
File.AppendAllLines(fileName, new[] { logMessage });
}
}
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Yeah thanks a lot - my initial class was like this but then I added buffer thinking to increase efficiency of writing and decrease time for threads waiting for each other.
So what are shortcomings of this final version of the class you think? Just performance? You said also it is fine to be used by many processes because AppendAllText uses FileShare.None flag?
|
|
|
|
|
user20044 wrote: AppendAllText uses FileShare.None flag?
AppendAllLines uses FileShare.Read. That will allow other processes to read the file, but not write to it.
user20044 wrote: So what are shortcomings of this final version of the class you think?
As previously mentioned, if multiple processes try to write to the same file at the same time, you'll get an exception, and the log entry from the second process will be lost.
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
No other shortcomings?
Also like in that thread will there be no issues with accessing the logDir variable in a thread safe way?
What about performance?
|
|
|
|
|
user20044 wrote: accessing the logDir variable in a thread safe way?
The changes I've made to the code deal with that. The field is only read once, so if someone changes it after that, you'll still be using the original value.
user20044 wrote: What about performance?
Try it and see. The only way to know is to measure the performance, which you can only do by running the code that's going to call this class.
user20044 wrote: No other shortcomings?
I'm sure lots of people would pick the code apart and find various problems with it - as they would with almost any non-trivial code. But if it works and it's good enough for your requirements, does it really matter?
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
I will keep this as option.
Now about performance I said since I log only errors maybe this will survive.
I'm sure lots of people would pick the code apart and find various problems with it - as they would with almost any non-trivial code. But if it works and it's good enough for your requirements, does it really matter?
I will keep this as option for this class also seemed as a possibility to use but people started panicking when looking at it saying don't create your logging class etc.
|
|
|
|
|
Please also change this class so that user can set base directory
|
|
|
|
|
Replace:
private const string LogDirectory = @"C:\Logs\";
with:
private static string _logDirectory = @"C:\Logs\";
public static string LogDirectory
{
get
{
return _logDirectory;
}
set
{
if (string.IsNullOrWhiteSpace(value))
{
throw new ArgumentNullException("value");
}
if (!value.EndsWith(@"\"))
{
value += @"\";
}
_logDirectory = value;
}
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
There are some good points on that StackExchange thread - particularly points 1 and 3 in Nikita Brizhak's answer, which address some incorrect behaviour in the class.
Fixing point 1 is easy - copy the _logDir field to a variable at the start of the method, and only use that variable:
string logDir = _logDir;
if (!logDir.EndsWith(@"\")) logDir += @"\";
Directory.CreateDirectory(logDir);
var todaysLogFilePath = Path.Combine(logDir, ...);
Fixing point 3 is slightly harder. You need to make sure that any entries in the buffer are saved when the application exits, regardless of the number of entries. To do that, you'll need to split the code that saves the entries from the code that tests the number of entries, as EBrown suggested.
static class GMLogger
{
private const int MaxBufferSize = 25;
private static readonly object _syncObject = new object();
private static readonly List<string> _buffer = new List<string>();
private static string _logDir = @"C:\Logs\";
public static string LogDirectory
{
get
{
return _logDir;
}
set
{
if (string.IsNullOrWhiteSpace(value))
{
throw new ArgumentNullException("value");
}
if (!value.EndsWith(@"\"))
{
value += @"\";
}
_logDir = value;
}
}
private static string EnsureLogDirectory()
{
string path = LogDirectory;
Directory.CreateDirectory(path);
return path;
}
private static string BuildLogPath(string directory, DateTime logDate)
{
string fileName = string.Format("Log{0:yyyy-MM-dd}.txt", logDate);
return Path.Combine(directory, fileName);
}
private static void SaveLog()
{
if (_buffer.Count != 0)
{
string directory = EnsureLogDirectory();
string fileName = BuildLogPath(directory, DateTime.Today);
File.AppendAllLines(fileName, _buffer);
_buffer.Clear();
}
}
public static void Log(string logMessage)
{
lock (_syncObject)
{
_buffer.Add(logMessage);
if (_buffer.Count == MaxBufferSize)
{
SaveLog();
}
}
}
public static void Save()
{
lock (_syncObject)
{
SaveLog();
}
}
}
You'd then need to make sure your Main method calls GMLogger.Save() before it returns. A finally block would probably be the best place for that:
static void Main()
{
try
{
...
}
finally
{
GMLogger.Save();
}
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Thanks one way is I remove buffer altogether and directly write to file what do you think? I had buffer to increase performance, but since I log only errors, maybe performance will not be problem?
Then I will take into account your advice about _logDir with temp variable. And class should be ready to be used?
Anyway, thanks for the feedback and discussion!.
|
|
|
|
|
|
First off, I would use a dedicated thread to do the logging. But just in case you don't want that, you should not lock while in the process of saving
List<string> copy = null;
lock(_syncObject) {
_buffer.Add(logMessage);
if ( _buffer.Count > _maxBufferSize) {
copy = _buffer;
_buffer = new List<string>();
}
}
if ( copy != null)
Save(copy);
(you would need to adjust your Save() function as well.
LATER EDIT: you will probably need a second lock for saving, but the main idea is that other threads would not be blocked when the buffer gets full.
Best,
John
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
