.NET locking IO code

Question

I have several Client objects (TCPClient wrappers) operating on separate threads. If any of these objects encounters a problem an error message is saved to an XML error log. Obviously file access is restricted to one process at a time so I need a way of preventing other threads from reading/writing while another is using it.

I'm currently using the lock method however an exception is still thrown that another process is using the file. I was under the impression lock will manage waiting and retrying.

// Lock the XML IO for safety due to multi-threading
lock (this.xmlDoc) // Changed from this to the xmlDoc
{
    // Attempt to load existing xml
    try
    {
        this.xmlDoc.Load(this.logPath);
    }
    catch (FileNotFoundException e)
    {
        // xml file doesn't exist, create
        this.xmlDoc.AppendChild(this.xmlDoc.CreateElement("root"));
    }
    // Get the doc root
    XmlElement root = this.xmlDoc.DocumentElement;
    // Create message entry
    XmlElement msg = this.xmlDoc.CreateElement("message");
    // Add <time></time> to msg
    msg.AppendChild(this.xmlDoc.CreateElement("time")).InnerText = dt.ToString();
    // Add <error></error> to msg
    msg.AppendChild(this.xmlDoc.CreateElement("error")).InnerText = message;
    // Add msg to root
    root.AppendChild(msg);
    // Save. Done.
    this.xmlDoc.Save(this.logPath);
}

Slaks I believe he was http://stackoverflow.com/questions/251391/why-is-lockthis-bad?rq=1 — MethodMan, Aug 16 '13 at 13:13
I was originally locking this - I understand why it's bad practice - however I get the same result when specifically locking the XmlDocument. @SLaks make a constructive comment next time. — Lee, Aug 16 '13 at 13:13
@Lee: Actual logging frameworks like log4net solve this and many other problems, and are probably more efficient than a manual lock. — SLaks, Aug 16 '13 at 13:16
@Slaks I'm sure they are, but I doubt my simple logging requirements warrant the use of an additional framework, besides, developers should not substitute understanding underlying logic with frameworks, which I'd be doing if I didn't ensure I know how to use lock properly. — Lee, Aug 16 '13 at 13:20
yes, that's better. I would move it to a seperate class, too. If it's critical for your perfomance you can use queues.. — Bjoern, Aug 16 '13 at 14:15
@JeremiahDotNet: There is nothing wrong with locking on a mutable type. — SLaks, Aug 16 '13 at 15:19
@Lee One unseen problem of your approach is that appending on the log is O(n), with n the current size of the log. While normal text logger can simply go at the end of the file and write, your log has to read the entire XML, modify it and rewrite it entirely. It's slow the log grows a little too much. You know this, right? — xanatos, Aug 16 '13 at 17:35
@xanatos that's a good point, although it's hard to miss when you load the xml doc every time the method is called. This has given me something to think about. Is there away to do a "blind append" so-to-speak with xml? — Lee, Aug 17 '13 at 19:59

score 4 · Answer 1 · answered Aug 16 '13 at 14:22

lock does not know what a file is. It operates on CLR objects, which exist per process. A different process will see a different object.

You need some cross-process mutual exclusion. Here are some options:

A named Mutex
File-level locking and a retry strategy (I suspect log4net does it that way)
Multiple files with different names (use random names so you don't get collisions)

Option 3 is the easiest.

score 0 · Answer 2 · answered Aug 16 '13 at 14:36

If all your threads are coming from the same process:

Use System.Diagnostics.Trace. Its built in to the framework, contains thread safety, and you can use a config file to define a trace listener.

Construct an XML string and then Trace.WriteLine().

If your threads are coming from different processes, each process will need to maintain its own log file as @usr mentions.

As a side note, you could log errors to the windows event viewer if the volume is low enough to avoid file locking issues.

.NET locking IO code

2 Answers2