My program needs to execute a series of requests. For performance, I decided to use an ActionBlock and perform most of them in parallel. Also the application uses a static class for logging that both writes to console and to a file. The problem is that even though I added a SemaphoreSlim to the logger, it appears the different threads used by the action block are not respecting it and every once in a while I get an exception for trying to access the log file while it's in use. How do you properly synchronize the actions of the action block? Here's a simplified version of my code:

private static async Task DoStuffWithThings(List<string> things)
{
  var blockOptions = new ExecutionDataflowBlockOptions
  {
    MaxDegreeOfParallelism = 50
  };
  var workerBlock = new ActionBlock<string>(DoStuff, blockOptions);

  foreach (var t in things)
  {
    workerBlock.Post(t);
  }

  workerBlock.Complete();
  await workerBlock.Completion;
}

private static void DoStuff(string str)
{
  LogMgr.Log($"Doing something with '{str}'...");
  DoSomeLongRequest(str);
  LogMgr.Log($"First part done. Doing something else with '{str}'...");
  DoAnotherLongRequest(str);
  LogMgr.Log($"Finished with '{str}'.");
}

And this is what the logger looks like:

public static class LogMgr
{
  private static readonly SemaphoreSlim semaphore = new SemaphoreSlim(1);
  
  public static void Log(string msg)
  {
    semaphore.Wait(1);
    var sw = new StreamWriter($"log.txt", true);
    sw.WriteLine(msg);
    sw.Close();
    semaphore.Release();
  }
}

Can someone help me understand why the SemaphoreSlim is not preventing different threads from accesing the file at the same time?

CodePudding user response：

You're providing a 1 millisecond timeout to the wait, so if the operation takes longer than one millisecond, you just let another thread into your critical section to do the work concurrently.

Either don't provide any timeout, or look at the return value of Wait to see if you were in fact granted the lock, and only doing the work in question if that's true.

While not the cause of your bug, you should also use a try/finally to ensure that if you did in fact take out the lock, you always call Release, even in the event of an error, else an exception in writing to this file will leave the lock held onto forever. You should put the StreamWriter in a using to ensure it's disposed of in the event of an error as well (or better yet, use File.AppendLine to avoid the problem entirely).