Making my Log Class THREAD SAFE? [CS/C# 2005]

S

Shaitan00

Junior Contributor
Joined
Aug 11, 2003
Messages
358
Location
Hell
I have a class (Log) that I use to log all my applications information into a single log file (text file), this class is accessed from many different threads all wanted to write to the same log file using the same Log.Trace(sMessage); static function...
Thing is - and correct me if I am wrong - but the way I am doing it seems extremely prone to contention, errors, and just generally not thread safe...

Currently this is the code I use to peform all my logging operations....
Code: 
public class Log
{
	static StreamWriter ErrorLog;
      
	public Log() {}

	public static void LogStart(string sFile)
	{
          // Open Tracer Log File
          ErrorLog = new StreamWriter(File.Open(sFile, FileMode.Append, FileAccess.Write, FileShare.ReadWrite));
	}

	public static void Trace(string sMessage)
	{
          // Write Trace Log
	  ErrorLog.WriteLine(sMessage);
          ErrorLog.Flush();
	}

	public void Close()
	{
           // Close Tracer Log File
           ErrorLog.Close();
	}
}

As you can see this is absolutly not threadsafe...
Somehow I have to make this safe enough so that all my threads can use this class to write to the same log file (and I have multiple concurrent running threads...)
Something like CRITICAL SECTIONS or .. arg .. I am totally unsure how to accomplish this ...
And hopefully I can make the change in the Log class and not have to modify the way I acctually call Log.Trace(""); (mind you that is the least of my worries seriously)...

Any ideas, hints, and help would be greatly appreciated, thanks
 
Just create an object that is used only for locking. It's generally a good idea to use the "readonly" keyword on the object so as to avoid accidently reassigning the object and messing up other things that are locking on it.
C#: 
class ThreadSafe
{
    static readonly object _lock = new object();

    public static void Test()
    {
        lock (_lock)
        {
            // critical section
        }
    }
}
This will automatically make the "Test" method thread-safe to all calling threads.


Edit: I made the _lock object into a static object. This is vital so that all calls to the "Test" method are locking on the same object.
 
Last edited:
As simple as that?
Why not lock(ThreadSafe)? [or in my case lock(typeof(Log))]?
Wouldn't that work better?

Or is it best to use OBJECT for some reason?
 
Yeah, that's pretty much all you need to do.

You can get away with locking on a lot of different stuff as long as the object you are locking on will be the same object across all threads for which you want to be mutually exclusive. If you look at a lot of threading code you will see that most people do it the way I did in my example. It's just easier to follow. By making the object readonly you eliminate the risk of assiging a new object to the member variable.
 
Microsoft recommends that you never lock certain types of objects (and with good reason). You should never lock an object unless it is completely within your control.

Shaitan00 said:
or in my case lock(typeof(Log))
Click to expand...
You should never lock a type. This object is managed by the CLR. Bad things can happen.

You should also never lock "this" unless the object locking "this" is a private class.

You should also never lock strings because the are interned by the CLR.

Generally the best thing to lock is a System.Object devoted to a specific critical section.
 
Back
Top