Incrementing values in multi threaded application

[pnk]

Hi

I have an application which scans a folder for files and creates an XML file for each file in the folder by reading few data from the existing folder. This is a multi threaded application and the user has the option to define the number of threads that he wants to create each time the application runs. Currently, I read the existing file names in the folder into an array list and use an integer variable to scan through the array list. I am using Interlocked.Increment to increment the variable.

Each thread calls a function where, before increment, i assign the existing value in the variable to a local variable and the next line is the increment as below.

intFileIndexLocal = intFileIndex;
Interlocked.Increment(ref intFileIndex);

After this intFileIndexLocal is used by the current thread to get the file name from the array list. It seems to be working perfectly but at times it is failing as 2 threads access the same file and it throws up an exception and freezes when the second thread tries to create the output XML file. The exception is that the XML file is already in use by another process. The user runs the application with about 15-20 threads.

Any help on overcoming this will be of great help. The application is already in production and it is becoming a regular issue as it starts freezing when it hits this exception.

Thanks

Navin

[Arjay]

Many folks new to multithreading mistakenly believe that they only have to protect the writes of a shared resource. The fact is that the reads and writes both need to be protected.

This is one of the problems with your code.

intFileIndexLocal = intFileIndex;  // Error: unprotected read access.
Interlocked.Increment(ref intFileIndex);

You can use the Interlocked.Exchange method to read the value in a thread safe manner.

intFileIndexLocal = Interlocked.Exchanged(ref intFileIndex, intFileIndex);

Another issue could be how you write the file. Can you post the write file code?

[pnk]

Hi Arjay

Thanks for responding. I will try the change you had recommended.

Here is how I create the XML file. For few reasons, the user might process the same file again. So I first check for the existence of the file and if it exists, I try to delete the existing XML file:

if (File.Exists(strXmlPath + "\\" + strFileName + ".xml"))
File.Delete(strXmlPath + "\\" + strFileName + ".xml");

strXmlFilename = strXmlPath + "\\" + strFileName + ".tmp";
Xmlwriter = new XmlTextWriter(strXmlFilename, Encoding.UTF8);
Xmlwriter.Formatting = Formatting.Indented;
Xmlwriter.Indentation = 5;
Xmlwriter.WriteStartDocument();

//Then all the nodes in the XML file

Xmlwriter.WriteEndDocument();
Xmlwriter.Flush();
Xmlwriter.Close();
//This is rename the .tmp file to xml file
if (File.Exists(strXmlFilename))
{
File.Move(strXmlFilename, strXmlPath + "\\" + strFileName + ".xml");
}

At times the program fails during the delete command. At times the program fails during Xmlwriter = new XmlTextWriter line.

Let me know if you would need further info on this.

Thanks

[Mutant_Fruit]

Interlocked.Increment returns you the incremented value. The correct way to use this is:

int valueToProcess = Interlocked.Increment (ref globalValue);

That *guarantees* that no two threads can ever process the same value. You can have dozens of threads doing Interlocked.Increments and if you print out the value of "valueToProcess" you'll never have duplicates.

[pnk]

Hi

Thanks for responding. I will try this and will see if I run into this problem again.

Interlocked.Increment returns you the incremented value. The correct way to use this is:

int valueToProcess = Interlocked.Increment (ref globalValue);

That *guarantees* that no two threads can ever process the same value. You can have dozens of threads doing Interlocked.Increments and if you print out the value of "valueToProcess" you'll never have duplicates.

[pnk]

I have rolled out the application with the change you suggested. Currently it is on test environment. Hopefully everything would go without any issues. Thanks for posting your suggestions.

Interlocked.Increment returns you the incremented value. The correct way to use this is:

int valueToProcess = Interlocked.Increment (ref globalValue);

That *guarantees* that no two threads can ever process the same value. You can have dozens of threads doing Interlocked.Increments and if you print out the value of "valueToProcess" you'll never have duplicates.

[pnk]

Hi Arjay

Thanks for responding. I will try your recommendations.

Here is what I am doing to create the XML files. The user has reasons to process the same file again. So I first check for the existence of the XML file and delete it and then go on to create the new XML file.

if (File.Exists(strXmlPath + "\\" + strFileName + ".xml"))
       File.Delete(strXmlPath + "\\" + strFileName + ".xml");

strXmlFilename = strXmlPath + "\\" + strFileName + ".tmp";
Xmlwriter = new XmlTextWriter(strXmlFilename, Encoding.UTF8);
Xmlwriter.Formatting = Formatting.Indented;
Xmlwriter.Indentation = 5;
Xmlwriter.WriteStartDocument();

Now i go on to create the nodes in the XML file and then close the file.

Xmlwriter.WriteEndDocument();
Xmlwriter.Flush();
Xmlwriter.Close();
//This is rename the .tmp file to xml file
if (File.Exists(strXmlFilename))
{
    File.Move(strXmlFilename, strXmlPath + "\\" + strFileName + ".xml");
}

Please do let me know if you would need any further info on this.

Thanks

Many folks new to multithreading mistakenly believe that they only have to protect the writes of a shared resource. The fact is that the reads and writes both need to be protected.

This is one of the problems with your code.

intFileIndexLocal = intFileIndex;  // Error: unprotected read access.
Interlocked.Increment(ref intFileIndex);

You can use the Interlocked.Exchange method to read the value in a thread safe manner.

intFileIndexLocal = Interlocked.Exchanged(ref intFileIndex, intFileIndex);

Another issue could be how you write the file. Can you post the write file code?

[Arjay]

if (File.Exists(strXmlPath + "\\" + strFileName + ".xml"))
       File.Delete(strXmlPath + "\\" + strFileName + ".xml");

strXmlFilename = strXmlPath + "\\" + strFileName + ".tmp";
Xmlwriter writer = new XmlTextWriter(strXmlFilename, Encoding.UTF8);
writer.Formatting = Formatting.Indented;
writer.Indentation = 5;
writer.WriteStartDocument();

Probably what's happening is that XmlWriter isn't releasing the file handle quick enough before you attempt to access the file again.

To fix this issue, put the XmlWriter declaration inside a 'using' block.

using( var writer = new XmlTextWriter(strXmlFilename, Encoding.UTF8) )
{
  writer.Formatting = Formatting.Indented;
  writer.Indentation = 5;
  writer.WriteStartDocument();

  //
  // Additional writing
  //
}  // File automatically closed here

[pnk]

I added one more line after the .xml delete line, to check if there is any .tmp file existing. This could mean that already another thread is processing this file. Although with the change we have made in Interlocked.Increment, this should not come up. But still, didn't want to take the chance again.

I will now try using the "using" block so that the .tmp file will get closed at any cost. Thanks for your response.

Probably what's happening is that XmlWriter isn't releasing the file handle quick enough before you attempt to access the file again.

To fix this issue, put the XmlWriter declaration inside a 'using' block.

using( var writer = new XmlTextWriter(strXmlFilename, Encoding.UTF8) )
{
  writer.Formatting = Formatting.Indented;
  writer.Indentation = 5;
  writer.WriteStartDocument();

  //
  // Additional writing
  //
}  // File automatically closed here

[someuser77]

intFileIndexLocal = intFileIndex; // Error: unprotected read access.
Interlocked.Increment(ref intFileIndex);

By 'unprotected read access' you mean that two threads can read the same value, NOT that a thread can read the wrong value, right?
As I understand it reading primitive types (except long on 32 bit) is always thread safe. You will never get a 'half-written' value.

[Arjay]

It seems to be int is atomic in the latest spec, however I don't believe that has always been the case. It's a bit interesting that the Interlocked class supports int.

In addition to long, it appears that ulong, double and decimal are also non atomic.