CODEWITHSUNDEEP
c#asp.netthreadpool
Andy T
Andy T

Reputation: 9881

Partial work being done twice (ThreadPool.QueueUserWorkItem)

I have created a newsletter system that allows me to specify which members should receive the newsletter. I then loop through the list of members that meet the criteria and for each member, I generate a personalized message and send them the email asynchronously .

When I send out the email, I am using ThreadPool.QueueUserWorkItem.

For some reason, a subset of the members are getting the email twice. In my last batch, I was only sending out to 712 members, yet a total of 798 messages ended up being sent.

I am logging the messages that get sent out and I was able to tell that the first 86 members received the message twice. Here is the log (in the order the messages were sent)

No.  Member   Date
1.   163992   3/8/2012 12:28:13 PM
2.   163993   3/8/2012 12:28:13 PM
...
85.   164469   3/8/2012 12:28:37 PM
86.   163992   3/8/2012 12:28:44 PM
87.   163993   3/8/2012 12:28:44 PM
...
798.   167691   3/8/2012 12:32:36 PM

Each member should receive the newsletter once, however, as you can see member 163992 receives message #1 and #86; member 163993 received message #2 and #87; and so on.

The other thing to note is that there was a 7 second delay between sending message #85 and #86.

I have reviewed the code several times and ruled out pretty much all of the code as being the cause of it, except for possibly the ThreadPool.QueueUserWorkItem.

This is the first time I work with ThreadPool, so I am not that familiar with it. Is it possible to have some sort of race-condition that is causing this behavior?

=== --- Code Sample --- ===

    foreach (var recipient in recipientsToEmail)
    {
        _emailSender.SendMemberRegistrationActivationReminder(eventArgs.Newsletter, eventArgs.RecipientNotificationInfo, previewEmail: string.Empty);
    }


    public void SendMemberRegistrationActivationReminder(DomainObjects.Newsletters.Newsletter newsletter, DomainObjects.Members.MemberEmailNotificationInfo recipient, string previewEmail)
    {
//Build message here .....

//Send the message
            this.SendEmailAsync(fromAddress: _settings.WebmasterEmail,
                                toAddress: previewEmail.IsEmailFormat()
                                            ? previewEmail
                                            : recipientNotificationInfo.Email,
                                subject: emailSubject,
                                body: completeMessageBody,
                                memberId: previewEmail.IsEmailFormat()
                                            ? null  //if this is a preview message, do not mark it as being sent to this member
                                            : (int?)recipientNotificationInfo.RecipientMemberPhotoInfo.Id,
                                newsletterId: newsletter.Id,
                                newsletterTypeId: newsletter.NewsletterTypeId,
                                utmCampaign: utmCampaign,
                                languageCode: recipientNotificationInfo.LanguageCode);
        }

    private void SendEmailAsync(string fromAddress, string toAddress, string subject, MultiPartMessageBody body, int? memberId, string utmCampaign, string languageCode, int? newsletterId = null, DomainObjects.Newsletters.NewsletterTypeEnum? newsletterTypeId = null)
    {
        var urlHelper = UrlHelper();
        var viewOnlineUrlFormat = urlHelper.RouteUrl("UtilityEmailRead", new { msgid = "msgid", hash = "hash" });
        ThreadPool.QueueUserWorkItem(state => SendEmail(fromAddress, toAddress, subject, body, memberId, newsletterId, newsletterTypeId, utmCampaign, viewOnlineUrlFormat, languageCode));
    }

Upvotes: 8

Views: 1303

Answers (6)

Daniel Lorenz
Daniel Lorenz

Reputation: 4336

Are you sure the query you are running to get the list of members to send the email to does not have duplicates in it? Are you joining to another table? What you could do is:

List<DomainObjects.Members.MemberEmailNotificationInfo> list = GetListFromDatabase();
list = list.Distinct().ToList();

Upvotes: 3

remio
remio

Reputation: 1280

In your code sample, we can't see where your logging takes place.

Maybe the mehod that sends the email erronously thought that something wrong occured then, the system tried again, which could result in an email sent twice.

Also, as written in other answers and comment, I would check again that I don't get duplicated entries in the list of recipients, and test it in a non-parallel context.

Upvotes: 1

Kamyar Nazeri
Kamyar Nazeri

Reputation: 26474

Having 800+ threads running on the server is not a good practice! Although you are using a ThreadPool, threads are being queued on the server and run whenever old threads return back to the pool and release the resource. This might take several minutes on the server and many situations like Race Conditions or Concurrencies might happen during that time. You could instead queue one work item, over one protected list:

lock (recipientsToEmail)
{
    ThreadPool.QueueUserWorkItem(t =>
        {
            // enumerate recipientsToEmail and send email
        });
}

Upvotes: 2

RickNZ
RickNZ

Reputation: 18654

One common cause of tasks getting performed twice in code where you queue the task to a background thread is faulty error handling. You might double-check your code to make sure that if there's an error that you don't always retry, regardless of the type of error (some errors warrant a retry; others don't).

Having said that, the code you've posted doesn't include enough information to definitively answer your question; there are many possibilities.

FWIW, are you aware that the SmtpClient class has a SendAsync() method, that doesn't require the use of a separate worker thread?

Upvotes: 1

Yaur
Yaur

Reputation: 7452

If this code:

foreach (var recipient in recipientsToEmail)
{
    _emailSender.SendMemberRegistrationActivationReminder(eventArgs.Newsletter
    ,eventArgs.RecipientNotificationInfo, previewEmail: string.Empty);
}

matches what you are actually doing... you have an obvious bug. namely that you are doing a foreach but not using the returned value, so you will send the same email to eventArgs.RecipientNotificationInfo for each entry in recipientsToEmail.

Upvotes: 1

Tim M.
Tim M.

Reputation: 54359

Things to check (I'm assuming you have a way to mock the sending of emails):

  • Is the number of duplicate emails always exactly the same? What if you increase/decrease the number of input values? Is it always the same user IDs which are duplicated?
  • Is SendEmail() doing anything of significance? (I don't see your code for it)
  • Is there a reason that you aren't using the framework's SendAsync() method?
  • Do you get the same behavior without multithreading?

For what it's worth, sending bulk email from your own site--even when it is completely legitimate--is not always worth the trouble. Spam blocking services are very aggressive and you don't want your domain to end up blacklisted. Third party services remove that risk, provide many tools, and also manage this part of the process for you.

Upvotes: 1

Related Questions