Is this thread safe?

[me]

public void WriteToLog(string message)
{
StringBuilder bld = new StringBuilder();
bld.AppendFormat(null,"{0} {1}", DateTime.Now.ToString("yy-
MM-dd hh:mm:ss"),message);
StreamWriter sw = File.AppendText(m_LogFile);
sw.WriteLine(bld.ToString());
sw.Flush();
sw.Close();
}

MSDN says all static members in System.IO are thread safe, but here an
object of type StreamWriter is created from AppendText and I'm not
sure...... Do I need critical section or not?
Thanks!

 

[Peter Duniho]

public void WriteToLog(string message)
{
StringBuilder bld = new StringBuilder();
bld.AppendFormat(null,"{0} {1}", DateTime.Now.ToString("yy-
MM-dd hh:mm:ss"),message);
StreamWriter sw = File.AppendText(m_LogFile);
sw.WriteLine(bld.ToString());
sw.Flush();
sw.Close();
}

MSDN says all static members in System.IO are thread safe, but here an
object of type StreamWriter is created from AppendText and I'm not
sure...... Do I need critical section or not?

That depends on what sort of access to m_LogFile might occur.

The String class is immutable, so once the reference has been passed in,
even if other threads are reading from the class, that wouldn't affect the
thread executing this method. Getting the StreamWriter is thread-safe,
with File.AppendText() being a static method (in a static class, no less)
and so falls under the "thread-safe" qualification for static members.

But you use m_LogFile which is presumably a class member accessible by
multiple threads. At the very least, it ought to be volatile. I think
that being a normal reference, you probably don't need to lock around it..
Even if you were modifying it from some other thread, the issue there
would be getting an out-of-date reference (i.e. you get the reference,
then immediately after it changes), and if that's a possibility then
that's a synchronization issue that can't be dealt with simply with a lock.

All that said, why not use the File.AppendAllText() method instead? It
doesn't avoid whatever issue (if any) exists with m_LogFile, but it does
simplify the code a lot.

Pete

 

[me]

That depends on what sort of access to m_LogFile might occur.

The String class is immutable, so once the reference has been passed in,
even if other threads are reading from the class, that wouldn't affect the
thread executing this method. Getting the StreamWriter is thread-safe,
with File.AppendText() being a static method (in a static class, no less)
and so falls under the "thread-safe" qualification for static members.

But you use m_LogFile which is presumably a class member accessible by
multiple threads. At the very least, it ought to be volatile. I think
that being a normal reference, you probably don't need to lock around it.
Even if you were modifying it from some other thread, the issue there
would be getting an out-of-date reference (i.e. you get the reference,
then immediately after it changes), and if that's a possibility then
that's a synchronization issue that can't be dealt with simply with a lock.

All that said, why not use the File.AppendAllText() method instead? It
doesn't avoid whatever issue (if any) exists with m_LogFile, but it does
simplify the code a lot.

Pete

Thanks Pete,
using AppendAllText really simplified things. Cheers!