HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Writing to file in a thread safe manner

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
filewritingmannerthreadsafe

Problem

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();
    }

Solution

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.

Code Snippets

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());
                }
            }

        }
    }
}

Context

StackExchange Code Review Q#49158, answer score: 18

Revisions (0)

No revisions yet.