using a Lock on a log Writer to Prevent Errors

Question

I am downloading Images from the web using a foreach and tasks, and saving them locally. After doing this I then write to a log saying the file name downloaded etc. The problem is I foresee that two tasks may try to write to the log at the same time causing an error. I would like to be able to lock the Log if any other Task is writing to it but I'm not sure how? So far I have:

 int filesDownloaded = 0;
 foreach (var fileName in ListOfFileNames)
 {
     Task.Factory.StartNew(() =>
     {
         //Download File
     }
     lock (thislock)
     {
          Log.WriteLine(string.Format("Downloaded File: {0}", f.FullName), Log.Status.Success); 
     }
     filesDownloaded++;
 }

Do I need to lock the filesDownloaded variable, or as this is a simple ++ operation does it not matter?

EDIT

Static Log Class:

public enum Status
{
    Info,
    Error,
    Success
}

private static string Directory { get; set; }

public static void CheckandCreateLogDirectory(string directoryPath)
{
    if (!System.IO.Directory.Exists(directoryPath))
    {
        System.IO.Directory.CreateDirectory(directoryPath);
    }
    Directory = directoryPath;
}

public static void WriteLine(string writeLine, Status status)
{
    if (Directory == null)
    {
        CheckandCreateLogDirectory(".\\Log\\"); 
    }
    using (System.IO.StreamWriter objWriter = new System.IO.StreamWriter(Directory + "Log" + DateTime.Now.ToString("ddMMyyyy") + ".log", true))
    {
        objWriter.WriteLine(string.Format("[{0}] {1}\t{2}", DateTime.Now, status.ToString(), writeLine));             
    }
}

I'm not sure what your Log.WriteLine refers to but you should probably lock the log file and then release it when logging is done. See http://stackoverflow.com/a/3202085/668521 — Dumpen, Aug 01 '14 at 11:02
I understand the method and will probably implement it however how would I implement it as a lock? would I just use a while loop and have that method as the Boolean value? — JKennedy, Aug 01 '14 at 11:10

score 1 · Accepted Answer · answered Aug 01 '14 at 11:22

Looking at your code there are two things that come to mind:

1) Move your locking into the WriteLine() method - no point having to repeat the code every time you want to log something

private static object _lockMe = new object();
public static void WriteLine(string writeLine, Status status)
{
    lock(_lockMe)
    {
        if (Directory == null)
        {
            CheckandCreateLogDirectory(".\\Log\\"); 
        }
        using (System.IO.StreamWriter objWriter = new System.IO.StreamWriter(Directory + "Log" + DateTime.Now.ToString("ddMMyyyy") + ".log", true))
        {
            objWriter.WriteLine(string.Format("[{0}] {1}\t{2}", DateTime.Now, status.ToString(), writeLine));             
        }
    }
}

2) You're downloading the file in a worker thread, but updating the log once you've queued up the worker - this will be BEFORE the file has downloaded. As a result, I'd move the logging INTO the worker thread, as follows:

int filesDownloaded = 0;
foreach (var fileName in ListOfFileNames)
{
    Task.Factory.StartNew((path) =>
    {
        DownloadMyFile(path);
        Log.WriteLine(string.Format("Downloaded File: {0}",path), Log.Status.Success); 
        lock (thislock)
        {
            filesDownloaded++;
        }
    }, fileName);
}

Brilliant, Works a charm. Thank you for the insight – JKennedy Aug 01 '14 at 11:31 — JKennedy, Aug 01 '14 at 11:31

MaLio · Answer 2 · 2014-08-01T11:39:27.207

Do I need to lock the filesDownloaded variable, or as this is a simple ++ operation does it not matter?

Yes, but better still use System.Threading.Interlocked.Increment. This must be done in the scope of your task or via a continuation, else you are going to experience timing issues. (The download count is not going to match the files you are expecting in the destination)

Do not "lock" around your logging code (client). Protect access to the file in the logger (the Log class). Here I would recommend a System.Threading.Mutex in case more than one process attempts to write to a single file (as if you had more than one instance of the application running).

using a Lock on a log Writer to Prevent Errors

2 Answers2