c# multithreading and resource locks

etienne_marais

etienne_marais

Honorary Master
Joined
Mar 16, 2008
Messages
16,253
Reaction score
19,743
Location
Centurion
I have a static method on a static class (Program) that writes (text) to files.

This method is called from various threads running simultaneously.

I know this is not the correct solution for locking resources, but I am more curious as to why it does not work and what is actually happening.

The idea is that while the text file to be written to is in use the method sleeps until it is (or should have been) released by the OS.

If multiple thread simultaneously call this method, is it definitely true that this static method is invoked and executed in parallel ?

MessageThreadLogLocked is a static property on the static class Program.


Code: 
public static void WriteToFileMessageThread(string meterNumber, string message)
        {
            message = Environment.NewLine + DateTime.Now.ToString() + message;

            while (MessageThreadLogLocked)
            {
                Thread.Sleep(15);
            }

            string subdirectory1 = DateTime.Now.Year.ToString() + DateTime.Now.Month.ToString() + DateTime.Now.Day.ToString();
            string path = AppDomain.CurrentDomain.BaseDirectory + "\\Logs\\" + subdirectory1;

            if (!Directory.Exists(path))
            {
                Directory.CreateDirectory(path);
            }

            string subdirectory2 = "MeterLogs";
            string path2 = AppDomain.CurrentDomain.BaseDirectory + "\\Logs\\" + subdirectory1 + "\\" + subdirectory2;

            if (!Directory.Exists(path2))
            {
                Directory.CreateDirectory(path2);
            }

            MessageThreadLogLocked = true;

            string filepath = AppDomain.CurrentDomain.BaseDirectory + "\\Logs\\" + subdirectory1 + "\\" + subdirectory2 + "\\" + meterNumber + ".txt";
            if (!File.Exists(filepath))
            {
                using (StreamWriter sw = File.CreateText(filepath))
                {
                    sw.WriteLine(message);
                }
            }
            else
            {
                using (StreamWriter sw = File.AppendText(filepath))
                {
                    sw.WriteLine(message);
                }
            }

            Thread.Sleep(10);

            MessageThreadLogLocked = false;
        }
 
Last edited:
This is what happens when too many threads try to write to it at once:

Exception =

The process cannot access the file 'C:\Users\me\Desktop\Test\TestService1\Logs\2022417\MeterLogs\messageinstances.txt' because it is being used by another process.

(In this case meterNumber is "messageinstances")
 
You would need to lock a mutex before you access the file and release afterwards. As written, I can’t see how this would stop multiple threads from hitting the code between your lock = true and lock = false statements, which is what I assume you wanted to be mutually exclusive.

Unless there is something C#-ish going on that I’m missing of course.
 
In terms of what could go wrong. Imagine you have two threads waiting for a 3rd to finish. When the third thread sets the lock = false, the other two threads could progress in lockstep, both setting the lock = true and both running the critical section simultaneously.


Also a possibility - you set the lock = true and then lock = false after. The compiler may decide to elide the lock = true altogether since it gets set to false anyway. In C or C++ this could be fixed with a volatile, atomic or soft fence. I expect there are equivalents in C#, although this still wouldn’t be sufficient to fix it.

You really need a variable that is going to get set only for one thread to perform the lock (so a CAS typically), and barriers to prevent the code from being reordered.
 
cguy said:
You would need to lock a mutex before you access the file and release afterwards. As written, I can’t see how this would stop multiple threads from hitting the code between your lock = true and lock = false statements, which is what I assume you wanted to be mutually exclusive.

Unless there is something C#-ish going on that I’m missing of course.
Click to expand...
Thanks, I can see that:

while (MessageThreadLogLocked)
{
Thread.Sleep(15);
}

can already have been passed by a series of threads before the first one even hits:

MessageThreadLogLocked = true;

and then all of them sets the true flag and all of them executes the file access section.

Previously I had the while/sleep section just before setting the flag to true. Would I still run into the same problem if I moved the while/sleep section back to right before setting the flag ?

(I still see the problem with eg. 2 other thread sleeping until first is done and then simultaneously setting to true and simultaneously running critical section)
 
etienne_marais said:
Thanks, I can see that:

while (MessageThreadLogLocked)
{
Thread.Sleep(15);
}

can already have been passed by a series of threads before the first one even hits:

MessageThreadLogLocked = true;

and then all of them sets the true flag and all of them executes the file access section.
Click to expand...
Exactly.
etienne_marais said:
Previously I had the while/sleep section just before setting the flag to true. Would I still run into the same problem if I moved the while/sleep section back to right before setting the flag ?
Click to expand...
Yes, even then. It will be less likely to occur, but it still isn’t safe. You need some mechanism that will guarantee atomicity, where you effectively try to set the lock to true, but only one thread can succeed.
 
There are several ‘rules’ for using locks. I do extensive multithreaded stuff in c# and my advice is that never make the object you lock on (lock object) public. Create a logger type class with your code in it and encapsulate everything. Only this class has access to the lock object and manages the locking of the resource as needed.
 
Spacerat said:
There are several ‘rules’ for using locks. I do extensive multithreaded stuff in c# and my advice is that never make the object you lock on (lock object) public. Create a logger type class with your code in it and encapsulate everything. Only this class has access to the lock object and manages the locking of the resource as needed.
Click to expand...
In my example, ok to have a local variable (object locker = new object()) thus essentially private within the method, which is static and in a static class... ? (so only the method has access to the locker object even though the method and class are public)
 
Last edited:
Nothing wrong with making it static but why not create a logger where you can specify the file name or folder so you can reuse it. Then not static. But yes the lock object will be private either way
 
etienne_marais said:
I have a static method on a static class (Program) that writes (text) to files.

This method is called from various threads running simultaneously.

I know this is not the correct solution for locking resources, but I am more curious as to why it does not work and what is actually happening.

The idea is that while the text file to be written to is in use the method sleeps until it is (or should have been) released by the OS.
Click to expand...
You're not locking the file.
docs.microsoft.com

FileShare Enum (System.IO)

Contains constants for controlling the kind of access other operations can have to the same file.
docs.microsoft.com

etienne_marais said:
This is what happens when too many threads try to write to it at once:

Exception =

The process cannot access the file 'C:\Users\me\Desktop\Test\TestService1\Logs\2022417\MeterLogs\messageinstances.txt' because it is being used by another process.

(In this case meterNumber is "messageinstances")
Click to expand...
docs.microsoft.com

TextWriter.Synchronized(TextWriter) Method (System.IO)

Creates a thread-safe wrapper around the specified TextWriter.
docs.microsoft.com

etienne_marais said:
Would I still run into the same problem if I moved the while/sleep section back to right before setting the flag ?

(I still see the problem with eg. 2 other thread sleeping until first is done and then simultaneously setting to true and simultaneously running critical section)
Click to expand...
Most probably, like all append operations you risk losing the current file content.
Common approach is to read current content, back up, do your work and then write everything back out.

If you don't need a file look at Windows Event Log, or a pub/sub approach using Redis or C# Channels.
Since this is I/O work, I would definitely do it async.
 
DA-LION-619 said:
You're not locking the file.
docs.microsoft.com

FileShare Enum (System.IO)

Contains constants for controlling the kind of access other operations can have to the same file.
docs.microsoft.com


docs.microsoft.com

TextWriter.Synchronized(TextWriter) Method (System.IO)

Creates a thread-safe wrapper around the specified TextWriter.
docs.microsoft.com


Most probably, like all append operations you risk losing the current file content.
Common approach is to read current content, back up, do your work and then write everything back out.

If you don't need a file look at Windows Event Log, or a pub/sub approach using Redis or C# Channels.
Since this is I/O work, I would definitely do it async.
Click to expand...
Thanks, so in short I should use synchronized text writer to ensure only one thread at a time uses it, but I have to call it asynchronously so as not to block a running thread trying to use it ?

I used:

object blocker = new object()

lock(object)
{
... file access
}

This did not work as expected. I thought the block of code between braces will be sequentially accessed in the order called by various threads, but only one thread at a time will be running through that code in the block hence ensuring the file is not accessed simultaneously, this did not work, why ?
 
typed wrong, I did use lock(blocker) but I think even if lock does accept (object) it should have the same outcome as it reserves the code-block between braces for single access.
 
etienne_marais said:
Thanks, so in short I should use synchronized text writer to ensure only one thread at a time uses it, but I have to call it asynchronously so as not to block a running thread trying to use it ?

I used:

object blocker = new object()

lock(object)
{
... file access
}

This did not work as expected. I thought the block of code between braces will be sequentially accessed in the order called by various threads, but only one thread at a time will be running through that code in the block hence ensuring the file is not accessed simultaneously, this did not work, why ?
Click to expand...
No. You need to step one level back, in theory your code could be perfectly fine.

The file is on the file system, which is managed by the OS. You have no guarantee something else isn’t using the file at the same time,
i.e search indexing, anti-virus etc.
 
I am no pro at c# but what worked for me in the past is creating some running indicators.

for example
declare public boolean as example :

public bool isrunning = false;

if (isrunning == false)
{
isrunning = true;
// execute your code and when done
}
isrunning = false;

like I say I am no pro, but managed to tackle a similar issue in the past with the above example :P
 
Gozzy said:
I am no pro at c# but what worked for me in the past is creating some running indicators.

for example
declare public boolean as example :

public bool isrunning = false;

if (isrunning == false)
{
isrunning = true;
// execute your code and when done
}
isrunning = false;

like I say I am no pro, but managed to tackle a similar issue in the past with the above example :p
Click to expand...
Thanks for keeping the rest of us employed. :p
 
Thank you.

So I
DA-LION-619 said:
No. You need to step one level back, in theory your code could be perfectly fine.

The file is on the file system, which is managed by the OS. You have no guarantee something else isn’t using the file at the same time,
i.e search indexing, anti-virus etc.
Click to expand...
Thank You.

So in essence

using (StreamWriter sw = File.AppendText(filepath))

could fail at anytime if it was the OS holding the file with an exception, but in normal terms happens very seldom and due to my high usage the probability became higher that it was indeed the OS holding it back and not another thread (The exception only happened every couple of hours while the file was being written to at least once every 10 seconds).

So the TextWrite.Synchronized wrapper will not throw an exception if the OS is still holding the file but will wait until it becomes available ? Or should I have a mechanism to retry until no exception is thrown, or perhaps a mechanism to check availability ?
 
Last edited:
I noticed that in your method, you aren't closing the StreamWriter with .Close method after writing. That means you are leaving it open for the garbage collector to finalize and close the file which could lead to intermittent blocking.

Just some other things I picked up:
etienne_marais said:
Code: 
string filepath = AppDomain.CurrentDomain.BaseDirectory + "\\Logs\\" + subdirectory1 + "\\" + subdirectory2 + "\\" + meterNumber + ".txt";
            if (!File.Exists(filepath))
            {
                using (StreamWriter sw = File.CreateText(filepath))
                {
                    sw.WriteLine(message);
                }
            }
            else
            {
                using (StreamWriter sw = File.AppendText(filepath))
                {
                    sw.WriteLine(message);
                }
            }
Click to expand...
File.AppendText opens a file for append or creates one if it doesn't exist so you can take out the whole check and File.CreateText. Just leave File.AppendText.

etienne_marais said:
Code: 
            string subdirectory1 = DateTime.Now.Year.ToString() + DateTime.Now.Month.ToString() + DateTime.Now.Day.ToString();
            string path = AppDomain.CurrentDomain.BaseDirectory + "\\Logs\\" + subdirectory1;

            if (!Directory.Exists(path))
            {
                Directory.CreateDirectory(path);
            }

            string subdirectory2 = "MeterLogs";
            string path2 = AppDomain.CurrentDomain.BaseDirectory + "\\Logs\\" + subdirectory1 + "\\" + subdirectory2;

            if (!Directory.Exists(path2))
            {
                Directory.CreateDirectory(path2);
            }
Click to expand...
Likewise, Directory.CreateDirectory creates all subdirectories so the first Directory.CreateDirectory is unnecessary.

I see you are creating 1 log file per Meter - do you have a lot of Meters or few?

If you have few Meters, by threadlocking, you are stopping other Meters from being written to. Also re-opening the files for Append constantly is expensive so I would use Dictionary<string, StreamWriter> to store the opened StreamWriters and then lock those so something along the lines of:

C#: 
//Have a static internal field:

private static var logWriters = new Dictionary<string, StreamWriter>;


// In method
if(!logWriters.ContainsKey(meterNumber))
{
      lock(logWriters)
      {
           logWriters.Add(meterNumber, File.AppendText(filepath));   // This should be done in a try...catch block
      }
}

StreamWriter sw = logWriters[meterNumber];
lock(sw)
{
    sw.WriteLine(message);  // Should also be done in try...catch
}

If on the other hand, you have got a lot of Meters so you aren't constantly writing to the same log file, I would keep separate Append operations to open the file, but maybe just add try...catch for the exceptions to keep it lighter.
 
me_ said:
I noticed that in your method, you aren't closing the StreamWriter with .Close method after writing. That means you are leaving it open for the garbage collector to finalize and close the file which could lead to intermittent blocking.
Click to expand...
Thanks, is there a downside to keeping them open (i.e. if I keep them in a Dictionary as suggested) ? The service once stabilized will run continuously...
me_ said:
Just some other things I picked up:

File.AppendText opens a file for append or creates one if it doesn't exist so you can take out the whole check and File.CreateText. Just leave File.AppendText.
Click to expand...

me_ said:
Likewise, Directory.CreateDirectory creates all subdirectories so the first Directory.CreateDirectory is unnecessary.
Click to expand...
Great, some clutter to remove then :)
me_ said:
I see you are creating 1 log file per Meter - do you have a lot of Meters or few?
Click to expand...
Many (thousands, currently about 7000)
me_ said:
If you have few Meters, by threadlocking, you are stopping other Meters from being written to. Also re-opening the files for Append constantly is expensive so I would use Dictionary<string, StreamWriter> to store the opened StreamWriters and then lock those so something along the lines of:
Click to expand...

me_ said:
If on the other hand, you have got a lot of Meters so you aren't constantly writing to the same log file, I would keep separate Append operations to open the file, but maybe just add try...catch for the exceptions to keep it lighter.
Click to expand...
Many meters, different logfiles. Will definitely add the try catch in there (as opposed to from the calling thread).

Why are try/catch within the method lighter than try/catch from where it is called though ?

Will open streamwriters (in a Dictionary) not still be beneficial ?, different logfiles but still many writes overall, to ease OS file access / system calls ? Though they will then effectively stay open as long as the service is running...
 
Last edited:
You must log in or register to reply here.
Back
Top
Sign up to the MyBroadband newsletter
X