Thread Safe Code with System.IO StreamWriter class

Question

I would like to generate 1 to 0.1 million numbers to get a unique ID for my file name. To achieve that I created one static property by using lock as follows:-

 private static readonly object objLock = new object();
        private static int _statInt = 1;

        private static string statInt
        {
            get
            {
                lock (objLock)
                {
                    if (_statInt >= 100000)
                    {
                        _statInt = 1;
                    }
                    else
                    {
                        _statInt = _statInt + 1;
                    }
                    return "_" + _statInt.ToString();
                }
            }
        }

Note I don't want to generate unique id throw guid as it could be duplicate or combination of date time[I tried both it was creating duplicate]. And in my case if above method fails after 0.1 million, it is file for me.[As the above code I will use for unique file name, and file creates in a batch of around 5000, and after that it gets delete]

First Question Is the above code thread safe?

If yes then my second and original question starts from here:-

I am just keeping full code here to understand the issue better:-

class Program
    {
        static void Main(string[] args)
        {
            _Interfaceupload obj = new _Interfaceupload();

            for (int i = 0; i < 100000; i++)
            {
                Thread thrd = new Thread(obj.getFileNoDuplicate); 
                thrd.Start();

            }

            Console.ReadLine();
            Dictionary<string, string> obj0 = _Interfaceupload.objDic;
            Console.ReadLine();
        }
    }

    class _Interfaceupload
    {

        private static readonly object objLock = new object();
        private static int _statInt = 0;
        private static string _fileName = "C:/TEST/vikas";
        private static string _InitfileName = "C:/TEST/vikas";
        private static string statInt
        {
            get
            {
                lock (objLock)
                {
                    if (_statInt > 100000)
                    {
                        _statInt = 0;
                    }
                    else
                    {
                        _statInt = _statInt + 1;
                    }
                    return "_" + _statInt.ToString();
                }
            }
        }

        public static string stateDate
        {
            get
            {
                return "_" + DateTime.Now.Ticks.ToString() + "_" + System.Guid.NewGuid();
            }
        }

        public static Dictionary<string, string> objDic = new Dictionary<string, string>();

        public void getFileNoDuplicate()
        {
            try
            {
                //objDic.Add(_InitfileName + statInt, string.Empty);
                // _fileName = _InitfileName + stateDate;
                _fileName = _InitfileName + statInt;
                objDic.Add(FileManager2.Write(_fileName, "txt", "hello", false), string.Empty);
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
            }
        }
    }

class FileManager2
    {
        public static string Write(string file, string ext, string data, bool overwrite)
        {
            return (string)OperateOnFile(file, ext, data, overwrite);
        }

        private static object OperateOnFile(string file, string ext,
           string data, bool overWrite)
        {
            StreamReader sr = null;
            StreamWriter sw = null;
            string workingFile = null;
            string dir = null;

            try
            {
                workingFile = file + "." + ext;
                if (overWrite == false && File.Exists(workingFile))
                {
                    workingFile = (string)OperateOnFile(workingFile + System.Guid.NewGuid().ToString(), ext, data, overWrite);
                }
                else
                {
                    dir = "C:/TEST/";
                    if (!Directory.Exists(dir))
                        Directory.CreateDirectory(dir);

                    sw = new StreamWriter(File.Open(workingFile, FileMode.Create, FileAccess.Write, FileShare.None), Encoding.UTF8);
                    sw.Write(data);
                }
                return workingFile;
            }
            finally
            {
                if (sr != null)
                    sr.Close();

                if (sw != null)
                {
                    sw.Flush();
                    sw.Close();
                }

            }
        }
    }

(may require following namespaces to include)

using System.Threading;
using System.IO;

Second Question : When I am running it in Console Application (.NET framework 4.5), for 0.1 million records, it looks that file is getting duplicate as I am getting exception "file is using by another process", however if the first code is thread safe then it should not create duplicate id till 0.1 million. What's wrong here, the way I am calling it? or issue with StreamWriter class or code is bypassing thread? not sure please advise.

Note I can't lock File.Exists method.

Edited After MarvinSmit's comment made methods as non static

  private string _fileName = "C:/TEST/vikas";
            private string _InitfileName = "C:/TEST/vikas";

Also removed the Dictionary and modified it as follows:-

public void getFileNoDuplicate()
        {
            try
            {
                //objDic.Add(_InitfileName + statInt, string.Empty);
                // _fileName = _InitfileName + stateDate;
                _fileName = _InitfileName + statInt;
                FileManager2.Write(_fileName, "txt", "hello", false);
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
            }
        }

After this also didn't work.

Solution

OMG!! I got the culprit... problem with the following line of code

_fileName = _InitfileName + statInt;

I have removed this line, and directly passed it to the method.

public void getFileNoDuplicate()
        {
            try
            {
                FileManager2.Write(_fileName + statInt, "txt", "hello", false);
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
            }
        }

It was the crazy mistake, I have created a single instance of the class and pass it to across the thread,

I appreciate Marvin's comment,

"_fileName = _InitfileName + statInt; being read without locking while being written to in a thread safe manner may lead to duplicates."

he noticed it very soon, but we both gone to different direction later.

Thanks for everyone's comment here

Yes, it is thread safe. It is not "process safe". In other words, Bad Things happen when you run your program more than once. Doesn't have to run concurrently either to cause this exception. Using the System.Guid class is a simple way to avoid filename collisions. — Hans Passant, Apr 30 '14 at 12:32
@HansPassant I am sorry, "process safe" , please clear what it means — vikas, Apr 30 '14 at 12:34
_fileName = _InitfileName + statInt; being read without locking while being written to in a thread safe manner may lead to duplicates. (i.e thr1 = counter++; thr2=readcounter(),thr3=readcounter() => duplicate read. Ps; look at Interlocked.Increment(ref x) for threadsafe counting (easier). — Marvin Smit, Apr 30 '14 at 12:36
@MarvinSmit your point looks valid to me, just hoping if we keep _filename in a lock, or keep it as non static it should work right? — vikas, Apr 30 '14 at 12:42
You have other problems (like using a non concurrent dictionary in your multi-threaded code). I would look for a way of making x threads run the processing, getting a unique filename at the start of their run. Also consider using the ThreadPool or Task library to perform these tasks instead of the low level Thread class. These give a lot of options (like signalling and waiting) you'd otherwise have to implement yourself (ManualResetEvent class and the likes) — Marvin Smit, Apr 30 '14 at 12:52
@MarvinSmit I have removed the `dictionary` (I was using it for debugging), and also made `_fileName` and `_InitfileName` as non static, it is not working, So I had written `ThreadPool` class also, and had checked it previously, it was not working, but as we just changed these variables as non static, so it may work now, let me try it once by using ThreadPool — vikas, Apr 30 '14 at 12:58
@MarvinSmit I had used ThreadPool also, but it is not going to help, and even as specified by Gusdor, I had compromised with 65000, not 100000 now — vikas, Apr 30 '14 at 13:27
You have to make `_fileName` local to the method, not a member of the class. So you'll have: `_fileName = _InitfileName + statInt;` — Jim Mischel, Apr 30 '14 at 13:30
@JimMischel that way we can achieve, I think you are saying that we can pass the value of filename in the method from calling code right? — vikas, Apr 30 '14 at 13:42
after so many comments, I still in doubt of my first question "Is the code thread safe?"\ — vikas, Apr 30 '14 at 13:45
Your `statInt` property is thread-safe. The rest of your code suffers from not understanding the differences between static fields, instance fields, and local variables, and therefore is frighteningly not thread-safe. — Jim Mischel, Apr 30 '14 at 14:20
@JimMischel `_fileName` and `_InitfileName` are not static now and even I am not using static instance of `Dictionary` also — vikas, Apr 30 '14 at 14:50
@JimMischel also not using `stateDate` also which I have taken static, and also there are only two static variables now `objLock` and `_statInt` — vikas, Apr 30 '14 at 14:53

score 0 · Answer 1 · edited Jun 20 '20 at 09:12

0

It doesn't matter how many locks you put around your FileStream a file cannot be opened more than once at the same time. This includes other applications that may have the file open. The exception you are receiving is telling you that you are trying to open the same file more than once - you path randomization code is broken.

There are exceptions - files that are opened with an explicit file share level.

Every call you make to FileManager2.OperateOnFile tries to open the file. Each call is one a different thread.

sw = new StreamWriter(File.Open(workingFile, FileMode.Create, FileAccess.Write, FileShare.None), Encoding.UTF8);

Is there any reason you can't use System.IO.Path.GetTempFileName? It is designed for exactly your situation.

Creates a uniquely named, zero-byte temporary file on disk and returns the full path of that file.

http://msdn.microsoft.com/en-us/library/system.io.path.gettempfilename%28v=vs.110%29.aspx

edited Jun 20 '20 at 09:12

Community

1
1

answered Apr 30 '14 at 12:34

Gusdor

14,001
2
52
64

I didn't lock the FileStream, the main concern here is I am passing unique file name, and why FileStream throwing this error – vikas Apr 30 '14 at 12:36
yeah its on different thread, and then I am calling statint to get unique file name right, so what is the issue? – vikas Apr 30 '14 at 12:39
@vikas the issue is that you are not generating a unique filename. see my updates – Gusdor Apr 30 '14 at 12:41
I can use `System.IO.Path.GetTempFileName`, but look into later if it is not achievable by my own logic :) – vikas Apr 30 '14 at 12:48
just reading the article you shared, it looks that it is valid for only 65535? >> The GetTempFileName method will raise an IOException if it is used to create more than 65535 files without deleting previous temporary files. – vikas Apr 30 '14 at 12:53
@vikas seems reasonable - 100,000 concurrent threads isn't going to achieve anything for you. You have far less cores in your processor and your hard disk probably cant do that many concurrent writes. `System.Threading.Parallel.For` will schedule the work and create the threads for you. It can be restricted to a maximum of 65535 concurrent operations with this solution -> http://stackoverflow.com/a/15931412/286976 – Gusdor Apr 30 '14 at 12:58

score 0 · Answer 2 · answered Apr 30 '14 at 13:01

I think your problem is here:

You decalre _fileName as static

private static string _fileName = "C:/TEST/vikas";

Then you have multiple threads assigning a value to it here:

public void getFileNoDuplicate()
{
    try
    {
        //objDic.Add(_InitfileName + statInt, string.Empty);
        // _fileName = _InitfileName + stateDate;
        _fileName = _InitfileName + statInt;
        objDic.Add(FileManager2.Write(_fileName, "txt", "hello", false), string.Empty);
    }
    catch (Exception ex)
    {
        Console.WriteLine(ex.Message);
    }
}

It is possible that one thread will set it, then another thread will set it again, then both threads will try to write to the same file. Drop the static and you'll be off in the right direction.

However, I agree with Gusdor that you should look into Parallel.For() and Path.GetTempFileName().

I had made changes to _fileName, made it static, but didn't help, and also I don't want to move to 4.0+ solution, as we have use it for previous framework code also, but I got the culprit now, the problem with the way I was assigning it, so I made those changes and now not only for 65000 or something, it is working for 100000 also :) — vikas, May 02 '14 at 10:16

score 0 · Answer 3 · answered Apr 30 '14 at 13:06

0

I think you can achieve what you want by using the following code (.Net 4+)

    private static string _basePath = "C:/TEST/vikas/";

    static void Main(string[] args)
    {
        Parallel.For(0, 10000, (index) =>
            {
                string filename = Path.Combine(_basePath, "_" + index.ToString());
                // filename is unique
            }
        );
    }

Hope this helps.

answered Apr 30 '14 at 13:06

Marvin Smit

4,088
1
22
21

I don't want to move to 4.0+ solution, as we have use it for previous framework code also, but I got the solution, I was doing a crazy mistake :). But finally got the culprit, thanks for your help – vikas May 02 '14 at 10:20
would you please mention your comment in your answer(to mark this post as an answer)- "_fileName = _InitfileName + statInt; being read without locking while being written to in a thread safe manner may lead to duplicates. (i.e thr1 = counter++; thr2=readcounter(),thr3=readcounter() => duplicate read. Ps; look at Interlocked.Increment(ref x) for threadsafe counting (easier). ", it was very helpful to find the solution. – vikas Jun 25 '14 at 16:13

Thread Safe Code with System.IO StreamWriter class

3 Answers3