9
\$\begingroup\$

I'm writing Stringbuilder to file asynchronously. This code takes control of a file, writes a stream to it and releases it. It deals with requests from asynchronous operations, which may come in at any time.

The FilePath is set per class instances (so the lock Object is per instance), but there is potential for conflict since these classes may share FilePaths. That sort of conflict, as well as all other types from outside the class instance, would be dealt with with retries.

Is this code suitable for its purpose? Is there a better way to handle this that means less (or no) reliance on the catch and retry mechanic?

Also how do I avoid catching exceptions that have occurred for other reasons?

public string Filepath { get; set; }
private Object locker = new Object();

public async Task WriteToFile(StringBuilder text)
    {
        int timeOut = 100;
        Stopwatch stopwatch = new Stopwatch();
        stopwatch.Start();
        while (true)
        {
            try
            {
                //Wait for resource to be free
                lock (locker)
                {
                    using (FileStream file = new FileStream(Filepath, FileMode.Append, FileAccess.Write, FileShare.Read))
                    using (StreamWriter writer = new StreamWriter(file, Encoding.Unicode))
                    {
                        writer.Write(text.ToString());
                    }
                }
                break;
            }
            catch
            {
                //File not available, conflict with other class instances or application
            }
            if (stopwatch.ElapsedMilliseconds > timeOut)
            {
                //Give up.
                break;
            }
            //Wait and Retry
            await Task.Delay(5);
        }
        stopwatch.Stop();
    }
\$\endgroup\$
  • \$\begingroup\$ Can you elaborate on what your requirements are? That will make it easier to not over-engineer this. \$\endgroup\$ – Ingo Bürk May 7 '14 at 19:56
  • \$\begingroup\$ There's a load test going on and every time a unit test is run it is logged. I think every unit test has it's own file so the lock on its own should be sufficient. If that's not the case, come to think of it, this try again method would probably be overwhelm and it would have to be a static lock. I think I'll just make it a simple lock and comment in my assumptions. \$\endgroup\$ – Nathan Cooper May 7 '14 at 20:31
15
\$\begingroup\$

I'm not sure why this is async.

Unless you have a good reason for write to the file asynchronously, this is wasted effort.

If you lock properly, this will work fine. Your locker variable should be marked static. You can then get away with something like this:

using System;
using System.IO;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace test
{   
    public class Writer
    {
        public string Filepath { get; set; }
        private static object locker = new Object();

        public void WriteToFile(StringBuilder text)
        {
            lock (locker)
            {
                using (FileStream file = new FileStream(Filepath, FileMode.Append, FileAccess.Write, FileShare.Read))
                using (StreamWriter writer = new StreamWriter(file, Encoding.Unicode))
                {
                    writer.Write(text.ToString());
                }
            }

        }
    }
}

This assumes you don't have some other reason for making this async.

\$\endgroup\$
  • 2
    \$\begingroup\$ By making the lock object static you lose the capability of writing to several files at once because all instances of Writer will have to wait, no matter what file they are writing to. \$\endgroup\$ – Ingo Bürk May 7 '14 at 19:56
  • 3
    \$\begingroup\$ If each instance of Writer points to a different file, then correct, you can use an instance locking object. If you have two instances of Writer pointing at the same file, the locking will do nothing unless you use the static lock. \$\endgroup\$ – David Crowell May 8 '14 at 13:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.