Parallel Foreach Loop Odd Behavior

Question

I am trying to send an email thousands of people and log it to database in a parallel foreach loop however some of the customers received an email that has different customers' name. How can i make it thread-safe? Here is my code:

EmailEntities db = new EmailEntities();
string templateData ="";
MailHelper mh = new MailHelper();
List<Customers> allCustomers = db.Customers.ToList<Customers>();

Parallel.ForEach(allCustomers, customer =>
{
    string[] emails = customer.EmailAddress.Split(';');

    foreach (var mailItem in emails)
   {

        string email = mailItem.Trim();
        templateData = mailEntitiyData.HtmlContent;             
        templateData = templateData.Replace("##FULL_NAME##", customer.Name + " " + customer.Surname);
        var postRes = mh.SendMail(subject, templateData , email, postType, null, null, bytes);
        Logger.Log(customer.ID, email, postRes.PostID);
   }

});


 public static class Logger
{
    public static void Log( int CustomerID, string email, string messageID)
    {
        using (EmailMarketingEntities db = new EmailMarketingEntities())
        {
            MailLogs ml = new MailLogs();
            ml.CustomerID = CustomerID;              
            ml.EmailAddress = email;
            ml.MessageID = messageID;               
            db.MailLogs.Add(ml);
            db.SaveChanges();
        }
    }

}

As per @Glubus comment, try defining `var templateData` inside lambda. Otherwise its value is used by different tasks = issue. — Sinatr, Apr 12 '16 at 13:26
mh is an MailHelper object which send the email and templateData is a string defined outside of the loop. I have edited the code. — Gökhan Yalçın, Apr 12 '16 at 13:31
As others have already highlighted, it's that `templateData` declaration. I can't see any reason why you'd have declared it outside of the inner loop, and you've got multiple threads fighting to set that single variable to new values and then (microseconds later) reading whatever the *current* value of it is and sending that to your `SendMail` function. — Damien_The_Unbeliever, Apr 12 '16 at 13:42
@Damien_The_Unbeliever Actually it was a standart foreach loop in the first place and then i converted it to parallel. That's why it is declared outside. That would resolve the issue. Thanks. — Gökhan Yalçın, Apr 12 '16 at 13:54

score 1 · Accepted Answer · answered Apr 12 '16 at 13:31

Using this:

foreach (var mailItem in emails)
{

    string email = mailItem.Trim();
    templateData = mailEntitiyData.HtmlContent;             
    templateData = templateData.Replace("##FULL_NAME##", customer.Name + " " + customer.Surname);
    var postRes = mh.SendMail(subject, templateData , email, postType, null, null, bytes);
    Logger.Log(customer.ID, email, postRes.PostID);
}

You're actually rewriting mailEntitiyData.HtmlContent content which is definitly not what you want to do. Change it to:

foreach (var mailItem in emails)
{

    string email = mailItem.Trim();
    templateData = mailEntitiyData.HtmlContent;             
    var customTemplateData = templateData.Replace("##FULL_NAME##", customer.Name + " " + customer.Surname);
    var postRes = mh.SendMail(subject, customTemplateData, email, postType, null, null, bytes);
    Logger.Log(customer.ID, email, postRes.PostID);
}

Should resolve your problem.

Parallel Foreach Loop Odd Behavior

1 Answers1