6

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();
}
Improve this question
7
  • Can you create a short, but complete, program that exhibits the problem? Commented Jan 8, 2010 at 19:29
  • @Lasse I will try the suggested solutions then post if still nothing. Commented Jan 8, 2010 at 19:51
  • Why can't you use SendAsync and just process the completion events so you know if all of the emails have been sent? Commented Jan 8, 2010 at 20:17
  • I think you missed some of the code. I don't see any call to send. Commented Jan 8, 2010 at 20:18
  • @Brian I want to send the alerts ASAP and they can be triggered pretty much 1 after the other, you are only allowed to send 1 asychronously at any 1 time, that was my original idea. Commented Jan 8, 2010 at 20:18

4 Answers 4

Reset to default
4

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).

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

But surely every thread is being signalled ok otherwise my application would wait forever?
Not if the WaitAll happens before all the threads have even had a chance to add their event to the dictionary. That's why it's important to initialize the list in the same thread that you wait in.
Ah very good point! I think will may have hit the nail on the head though.
Have you tried the updated version? I have a feeling that the dictionary access may have been part of the issue, it isn't thread-safe. Construct the list of events in the foreground thread, do away with the dictionary completely, and you should be fine.
Spot on, that seemed to solve the issue. Thanks for your time.
|
2

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());
Improve this answer

9 Comments

or ThreadPool.QueueUserWorkItem((state) =>{ events[(Guid)state].Set();}, id);
I did this so I don't have to ask if he has .NET 3.5.
For the record I do have 3.5, just trying your solution at the mo will get back to you guys!
Hmm, the var should have been a red flag for me. I need a vacation.
Same result, 1 email this time didn't arrive in the mailbox (i waited a little extra for it) then adding the thread sleep worked!
|
2

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.

Improve this answer

13 Comments

Of course this only uses one thread for all the alerts... they are still being sent sequentially instead of in parallel. If the objective is just to do it in the background then that'll work fine, but if he's trying to send them all in parallel then this doesn't do exactly the same thing.
I know, its just about pointless. Why do this asynchronously when you want to block?
I assumed that it was in order to improve overall throughput. Send() might take a relatively long time to execute, but if the mail server accepts 10 connections at the same time then the parallel version will finish much faster.
I want the alerts to go ASAP and I just want the application to wait for ALL alerts to have been sent. If the application ends too quickly then some alerts don't get sent hence why I need to wait.
Well, if alert.Send() blocks until sent (you'll have to create a demo project to test this), then just send them synchronously. If you want to prevent application shutdown from stopping them from getting sent, create a new Thread and use it to run your alerts (i.e., don't use a "background" thread; see here: msdn.microsoft.com/en-us/library/h339syd0.aspx )
|
0

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);
 }
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.