CODEWITHSUNDEEP
c#multithreadingsmtp
James
James

Reputation: 82136

Sending emails on separate threads using QueueUserWorkItem

I have a console application that sends customized emails (with attachments) to different recipients and I want to send them concurrently. I need to create separate SmtpClients to achieve this so I am using QueueUserWorkItem to create the emails and send them in separate threads.

Snippet 

var events = new Dictionary<Guid, AutoResetEvent>();
foreach (...)
{
    ThreadPool.QueueUserWorkItem(delegate
    {
        var id = Guid.NewGuid();
        events.Add(id, new AutoResetEvent(false));
        var alert = // create custom class which internally creates SmtpClient & Mail Message
        alert.Send();
        events[id].Set();
    });   
}
// wait for all emails to signal
WaitHandle.WaitAll(events.Values.ToArray());

I have noticed (intermittently) that sometimes not all the emails arrive in the specific mailboxes with the above code. I would have thought that using Send over SendAsync would mean the email has definitely sent from the application. However, adding the following line of code after the WaitHandle.WaitAll line:

System.Threading.Thread.Sleep(5000);

Seems to work. My thinking is, for whatever reason, some emails still haven't been sent (even after the Send method has run). Giving those extra 5 seconds seems to give the application enough time to finish.

Is this perhaps an issue with the way I am waiting on the emails to send? Or is this an issue with the actual Send method? Has the email definitely been sent from the app once we pass this line?

Any thoughts idea's on this would be great, can't quite seem to put my finger on the actual cause.

Update

As requested here is the SMTP code:

SmtpClient client = new SmtpClient("Host");
FieldInfo transport = client.GetType().GetField("transport", BindingFlags.NonPublic | BindingFlags.Instance);
FieldInfo authModules = transport.GetValue(client).GetType()
    .GetField("authenticationModules", BindingFlags.NonPublic | BindingFlags.Instance);
Array modulesArray = authModules.GetValue(transport.GetValue(client)) as Array;
modulesArray.SetValue(modulesArray.GetValue(2), 0);
modulesArray.SetValue(modulesArray.GetValue(2), 1);
modulesArray.SetValue(modulesArray.GetValue(2), 3);
try
{
    // create mail message
    ...
    emailClient.Send(emailAlert);
}
catch (Exception ex)
{
    // log exception
}
finally
{
    emailAlert.Dispose();
}

Upvotes: 6

Views: 7145

Answers (4)

robor
robor

Reputation: 3089

I had a similar problem (using SmtpClient from a thread and the emails arrive only intermittently).

Initially the class that sends emails created a single instance of SmtpClient. The problem was solved by changing the code to create a new instance of SmtpClient everytime an email needs to be sent and disposing of the SmtpClient (with a using statement).

 SmtpClient smtpClient = InitSMTPClient();
 using (smtpClient)
 {
    MailMessage mail = new MailMessage();
    ...
    smtpClient.Send(mail);
 }

Upvotes: 0

user1228
user1228

Reputation:

The reason why its not working is that when he hits events.Values.ToArray() not all of the queued delegates have executed and therefore not all AutoResetEvent instances have been added to the dictionary.

When you call ToArray() on the Values property, you get only those ARE instances already added!

This means you'll be waiting for only a few of the emails to be sent synchronously before the blocked thread continues. The rest of the emails have yet to be processed by the ThreadPool threads.

There is a better way, but this is a hack it seems pointless to do something asynchronously when you want to block the calling thread in the end...

var doneLol = new AutoResetEvent();

ThreadPool.QueueUserWorkItem(
delegate
{
  foreach (...)
  {
    var id = Guid.NewGuid();
    var alert = HurrDurr.CreateAlert(...);
    alert.Send();
  }
  doneLol.Set();
});   

doneLol.WaitOne();

Okay, considering the following requirements:

  1. Console App
  2. Lots of emails
  3. Sent as fast as possible

I'd create the following application:

Load the emails from a text file (File.ReadAllLines). Next, create 2*(# of CPU cores) Threads. Determine the number of lines to be processed per thread; i.e., divide the number of lines (addy per line) by number of threads, rounding up. Next, set each thread the task of going through its list of addresses (use Skip(int).Take(int) to divvy up the lines) and Send()ing each email synchronously. Each thread would create and use its own SmtpClient. As each Thread completes, it increments an int stored in a shared location. When that int equals the number of threads, I know all threads have completed. The main console thread will continuously check this number for equality and Sleep() for a set length of time before checking it again.

This sounds a bit kludgy, but it will work. You can tweak the number of threads to get the best throughput for an individual machine, and then extrapolate from that to determine the proper number of threads. There are definitely more elegant ways to block the console thread until complete, but none as simple.

Upvotes: 2

ChaosPandion
ChaosPandion

Reputation: 78302

You probably want to do this...

var events = new Dictionary<Guid, AutoResetEvent>();
foreach (...)
{
    var id = Guid.NewGuid();
    events.Add(id, new AutoResetEvent(false));
    ThreadPool.QueueUserWorkItem((state) =>
    {           
        // Send Email
        events[(Guid)state].Set();
    }, id);   
}
// wait for all emails to signal
WaitHandle.WaitAll(events.Values.ToArray());

Upvotes: 2

Aaronaught
Aaronaught

Reputation: 122684

One of the things that's bugging me about your code is that you call events.Add within the thread method. The Dictionary<TKey, TValue> class is not thread-safe; this code should not be inside the thread.

Update: I think ChaosPandion posted a good implementation, but I would make it even simpler, make it so nothing can possibly go wrong in terms of thread-safety:

var events = new List<AutoResetEvent>();
foreach (...)
{
    var evt = new AutoResetEvent();
    events.Add(evt);
    var alert = CreateAlert(...);
    ThreadPool.QueueUserWorkItem(delegate
    {           
        alert.Send();
        evt.Set();
    });
}
// wait for all emails to signal
WaitHandle.WaitAll(events.ToArray());

I've eliminated the dictionary completely here, and all of the AutoResetEvent instances are created in the same thread that later performs a WaitAll. If this code doesn't work, then it must be a problem with the e-mail itself; either the server is dropping messages (how many are you sending?) or you're trying to share something non-thread-safe between Alert instances (possibly a singleton or something declared statically).

Upvotes: 4

Related Questions