3
\$\begingroup\$

I'm using ConcurrentDictionary to hold the records.. I'm reading files from the local system in parallel, to speed up the process.

Sample 1:

static ConcurrentDictionary<Int64, Byte[]> concurrentDictionary = new ConcurrentDictionary<Int64, Byte[]>();

private void ReadFilesFromDirectory()
{
  string[] docNames = Directory.GetFiles(CompleteLettersDirectory);
  Parallel.ForEach(docNames, new ParallelOptions { MaxDegreeOfParallelism = 8 }, file =>
    {
      try
      {
          int64 letterId = Convert.ToInt64(file.Split('_')[1]);
          Byte[] CompleteLetterContent;
          CompleteLetterContent = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);
          concurrentDictionary.GetOrAdd(letterId, CompleteLetterContent);
      }
      catch (Exception ex)
      {
          //Logger.WriteFile(letterId.ToString() + Environment.NewLine, ex);
      }
    }
}

Sample 2: In this sample I want to keep multiple values in dictionary as such I created a letter class which has multiple properties. As Letter class is not threadsafe, will it create any issues ?

static ConcurrentDictionary<Int64, Letter> concurrentDictionary = new ConcurrentDictionary<Int64, Letter>();

private void ReadFilesFromDirectory()
{
  string[] docNames = Directory.GetFiles(CompleteLettersDirectory);
  Parallel.ForEach(docNames, new ParallelOptions { MaxDegreeOfParallelism = 8 }, file =>
    {
      try
      {
          int64 letterId = Convert.ToInt64(file.Split('_')[1]);
          Byte[] LetterContent;
          Byte[] CompleteLetter;

          Letter letter = new Letter();
          letter.LetterId = letterId;

      if(file.Contains("CL"))
      {
    letter.CompleteLetter = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);
      }
      else
      {
    letter.LetterContent = ConvertToHTML(System.IO.File.ReadAllBytes(file), letterId);
      }
    concurrentDictionary.GetOrAdd(letterId, letter);
          }
      catch (Exception ex)
  {
          //Logger.WriteFile(letterId.ToString() + Environment.NewLine, ex);
      }
    }
}

public class Letter
  {
      public Int64 LetterId { get; set; }
      public Byte[] LetterContent { get; set; }
      public Byte[] CompleteLetter { get; set; }
  }

Can anyone advise if code samples are threadsafe. Also, should string docNames which holds documents from the directory and is passed to Parallel.ForEach be also threadsafe or not? Or any other recommendation.

\$\endgroup\$
2
\$\begingroup\$

I'm reading files from the local system in parallel, to speed up the process.

Are you sure it's actually beneficial? Hard disks work best when you read them in sequence, not when they have to go back and forth to satisfy concurrent requests. (And even if you have a SSD, you're not going to gain anything.)


You seem to be in the habit of using framework names for types (e.g. Byte, Int64) instead of the C# names (byte, long). I believe that using the C# names is more common.


Byte[] CompleteLetterContent;
CompleteLetterContent = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);

There is no reason to separate the declaration and assignment here. Also, local variables are generally written in camelCase:

Byte[] completeLetterContent = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);

System.IO.File.ReadAllBytes(file)

You're already using System.IO, so the namespace should be redundant here. And if it isn't because you have something else called File, then you should probably fix that conflict.


concurrentDictionary.GetOrAdd(letterId, CompleteLetterContent);

You're calling GetOrAdd(), but you always only want to add, you never want to get. For that, you can either use TryAdd() or the indexer setter:

concurrentDictionary.TryAdd(letterId, CompleteLetterContent);
concurrentDictionary[letterId] = CompleteLetterContent;

In all three cases, the behavior is the same if there are no values with the same key, but I think the latter two are clearer.


      Byte[] LetterContent;
      Byte[] CompleteLetter;

These variables are never used, get rid of them.


Letter letter = new Letter();
letter.LetterId = letterId;

You can use var (since the type is clear from the assignment) and object initializer here:

var letter = new Letter { LetterId = letterId };

if(file.Contains("CL"))
{
    letter.CompleteLetter = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);
}
else
{
    letter.LetterContent = ConvertToHTML(System.IO.File.ReadAllBytes(file), letterId);
}

In both branches, you're repeating the System.IO.File.ReadAllBytes(file) part. Consider extracting it into a local variable before the condition.


The code you have shown appears to be thread-safe, assuming that the methods that you're calling from it (ConvertToPdfWithAspose, ConvertToHTML) are thread-safe.

\$\endgroup\$
1
\$\begingroup\$

The term "threadsafe" usually describes an object's behavior when its member functions are invoked from different threads. That means that the Letter class is too simple to be described as threadsafe or not threadsafe - it's just a dumb class with some public members. There isn't a problem writing to different fields, and even writing to the same field can only go so wrong - one value gets lost, but what did you expect? The way you combine the Letter and the ConcurrentDictionary is backwards, though. You need to do something like:

var letter = concurrentDictionary.GetOrAdd(letterId, new Letter());
if(file.Contains("CL"))
{
    letter.CompleteLetter = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);
}
else
{
    letter.LetterContent = ConvertToHTML(System.IO.File.ReadAllBytes(file), letterId);
}

In your code, if the key exists the new letter you made (and stored data in!) is dropped on the floor.

Use of docNames is fine.

\$\endgroup\$
  • \$\begingroup\$ If you use Parallel.ForEach() incorrectly, then even a single method can be not thread-safe. I think that's the question here. \$\endgroup\$ – svick May 24 '15 at 21: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.