0

I have an .ASMX API build using c#/asp.net/.net4.7.2 and it has been set to use 16 threads in IIS. There are various modules that write (only writing, no reading) to multiple log files like so:

2022-06-01-Wed-_ModuleA.log
2022-06-01-Wed-_ModuleB.log

I was using the usual lock() method:

public class Audit {
    public static object LogWriteLocker = new object();
    public void LogEntry(string path, string logText) {
        lock (LogWriteLocker) {
             File.AppendAllText(path, logText);
        }
    }
}

This should not be happening, but I was seeing errors like:

The process cannot access the file 'D:\MySite\App_Data\2022-06-01-Wed-_ModuleA.log' because it is being used by another process.

So I am trying to figure our a workaround like below:

readonly private static ReaderWriterLockSlim _readWriteLock = new ReaderWriterLockSlim();
public static void WriteToFileThreadSafe(string path, string text) {
    _readWriteLock.EnterWriteLock();
    try {
        File.AppendAllText(path, text);
    }
    catch (Exception exx) {
        LogException(exx, "Error Writing to Log");
    }
    finally {
        _readWriteLock.ExitWriteLock();
    }
}

Sorry if there is TMI, but my questions are:

  1. What is wrong with my implementation of the lock() method?
  2. Is ReaderWriterLockSlim implementation any better? I have not used this before.
  3. One problem is using a single lock object for multiple files, where as I should have an array of lock objects/ReaderWriterLockSlim. The number of log files are dynamic so how do I do that efficiently?

Thank you.

Edit:

Contemplating a 3rd option, that has separate locks for each file path:

private static ConcurrentDictionary<string, object> LogWriteLocker = new ConcurrentDictionary<string, object>();
public static void WriteToFileThreadSafe(string path, string text)
{
    //Adds a key/value pair to the ConcurrentDictionary<TKey,TValue> if the key does not already exist. Returns the new value, or the existing value if the key already exists.
    var lockObj = LogWriteLocker.GetOrAdd(path, new object());
    lock(lockObj)
    {
        File.AppendAllText(path, text);
    }
}
Vaibhav Garg
  • 3,630
  • 3
  • 33
  • 55
  • 1
    Is your `Audit` class a singleton, or do you have multiple instances of it? – Matthew Watson Aug 03 '22 at 08:40
  • 2
    Probably the error you get is not caused by your code. If you run your application with multiple worker processes, the in-process lock mechanism becomes useless. – Eldar Aug 03 '22 at 08:44
  • For the 3. case, you need separate locks for separate files. you can use a concurrent dictionary with the filenames as the keys and the locks as the values. – Eldar Aug 03 '22 at 08:53
  • @MatthewWatson It is used all over, multi instance – Vaibhav Garg Aug 03 '22 at 09:08
  • @Eldar I am not well versed so not sure what your reasoning is for the multi process macking lock useless. Good idea with the 'a concurrent dictionary with the filenames' but is the ReaderWriterLockSlim implementation other wise looking good for my usecase? – Vaibhav Garg Aug 03 '22 at 09:10
  • 2
    Actually, `ReaderWriterLockSlim` won't have any effect on your code. As you mentioned the only operation you perform is writing. `ReaderWriterLockSlim` separates from `lock` when you read and write in a multithreading manner. It allows you to use different locks when you read and write. So if the only operation is writing it will behave like `lock`. For an inter-process locking mechanism, you need to use something that is not in-process (The lock defined in a process won't block the other processes trying to access a shared resource (like a file)). You can use a global mutex or a lock file. – Eldar Aug 03 '22 at 09:18
  • Try adding pid to the path to see if this problem is fixed. – shingo Aug 03 '22 at 09:29
  • @Eldar I am not how a global mutex will help as it is primaraly for single instance apps as I understand it. Lock file, also wont work if I cant wait for it to release due to some reason. At he moment, lock() has rare errors – Vaibhav Garg Aug 03 '22 at 09:52
  • @Eldar I added a third option using ConcurrentDictionary. Does that seem like it should work? – Vaibhav Garg Aug 03 '22 at 10:05
  • 2
    @VaibhavGarg like I said any in-process solution won't work if multiple processes are accessing a shared resource. If you guarantee that a single process accesses the log files then the dictionary solution would perform better than the other options. – Eldar Aug 03 '22 at 12:05
  • @Eldar Ok thank you for sticking with the discussion, I have learned quite a lot and now know the issue was me looking at it as "multi-thread" where as it is a "multi process" situation. My lock() was doing nothing :) – Vaibhav Garg Aug 03 '22 at 12:11

2 Answers2

3

Looks like you are writing to the file across different processes. In which case you need a global mutex.

public class Audit
{
    public static Mutex LogWriteLocker = new Mutex(false, "Global\MyLoggingFileMutex");

    public void LogEntry(string path, string logText)
    {
        try
        {
            try
            {
                LogWriteLocker.WaitOne();
            }
            catch (AbandonedMutexException)
            { //
            }
            File.AppendAllText(path, logText);
        }
        finally
        {
            LogWriteLocker.ReleaseMutex();
        }
    }
}

The ReleaseMutex call must happen on the same thread as WaitOne, so if you use async you need to make sure to marshal back to the original thread using a SynchonizationContext.

Charlieface
  • 52,284
  • 6
  • 19
  • 43
  • That is exactly what I was just reading up on. As I have multiple files and I dont want to carry forward the limitation of using the same lock for all of them, do you this is good: `Mutex mutex = new Mutex(false, "Global\\" + path.Replace("\\", "")); ` – Vaibhav Garg Aug 03 '22 at 11:49
  • 1
    Careful the path isn't too long – Charlieface Aug 03 '22 at 12:00
  • I will use just the fileName then as I know it will never be too long in my case. Is it necessary to use Global? I want just the IIS processes for this website to share the mutex but it is not critical and I can use global. – Vaibhav Garg Aug 03 '22 at 12:06
  • 1
    `Global` is for different sessions, `Local` is the default, in other words just your session. Depends whether they are running in the same session/user as IIS – Charlieface Aug 03 '22 at 12:43
  • 1
    And remember that Windows' System process "owns" the reading / writing to the any file, so even if YourProcessA and YourProcessB have good synchronization for the reads / writes, these processes are still at the mercy of when System process is truly done with its work. So the best advice here, is to catch exception, and add retry logic (in addition to the proper locking). – Chris O Aug 03 '22 at 16:20
0

In my project I had about 10 processes that were writing to some common logs. I kept getting the UnauthorizedAccessException: Access to the path 'Global\mymutexid' is denied. intermittently. So I ended up changing the constructor as below (idea from here) :

public static bool WriteToFileProcessAndThreadSafe(string filePath, string fileName, string logText)
{
    //Mutex LogWriteLocker = new Mutex(false, "Global\\" + fileName); // causes UnauthorizedAccessException
    Mutex LogWriteLocker = null;

    try
    {
        MutexSecurity mutexSecurity = new MutexSecurity();
        mutexSecurity.AddAccessRule(new MutexAccessRule(new SecurityIdentifier(WellKnownSidType.WorldSid, null), MutexRights.Synchronize | MutexRights.Modify, AccessControlType.Allow));

        // attempt to create the mutex, with the desired DACL..
        //The backslash (\) is a reserved character in a mutex name.
        //The name can be no more than 260 characters in length.
        LogWriteLocker = new Mutex(false, "Global\\" + fileName, out _, mutexSecurity);

        try
        {
            LogWriteLocker.WaitOne();
        }
        catch (AbandonedMutexException amex)
        {
            //Handle AbandonedMutexException;
        }

        //We are now thread and process safe. Do the work.
        File.AppendAllText(Path.Combine(filePath, fileName), logText);
        
        return true;
    }
    catch (WaitHandleCannotBeOpenedException)
    {
        // the mutex cannot be opened, probably because a Win32 object of a different
        // type with the same name already exists.
        return false;
    }
    catch (UnauthorizedAccessException)
    {
        // the mutex exists, but the current process or thread token does not
        // have permission to open the mutex with SYNCHRONIZE | MUTEX_MODIFY rights.
        return false;
    }
    catch (Exception exx)
    {
        //Handle other exceptions
        return false;
    }
    finally
    {
        if(LogWriteLocker != null) 
        {
            LogWriteLocker.ReleaseMutex();
            LogWriteLocker.Dispose();
        }
    }        
}

I have finally understood that my mistake was thinking of this in terms of "milti-threading" were as the issue was due to "multi-process". All the variations I have listed in the OP end up with lock() at the core, which works only for single process, multi thread situation.

Thanks to @Eldar and just an addition to @Charlieface answer, here is what I am using for my cause. I use the fileName as mutex name so that each file has a separate lock (as opposed to using one lock for all files)

Vaibhav Garg
  • 3,630
  • 3
  • 33
  • 55