6
\$\begingroup\$

This is my first foray into multi-threading, and I'm keen to know if I have fallen into any traps or have room for optimisation/improvement.

Example usage:

var success = new ConcurrentBag<CourseParticipant>();
var fail = new ConcurrentBag<CourseParticipant>();
using (var parallelEmails = new ParallelSmtpEmails())
{
     foreach (var cp in courseParticipants)
     {
        var mail = new MailMessage{
            From = new MailAddress("from@example.com"),
            To = cp.email,
            Subject = "come and attend course",
            BodyFormat = MailFormat.Html
        }
        string token = await GetRegistrationTokenAsync(cp);
        mail.Body = $"<div><a href='example.com/register?userToken='{token}'>let me come</a></div>"
        parallelEmails.Send(mail, s=> 
        {
            if (s == null)
            {
                success.Add(cp);
            }
            else
            {
                fail.Add(cp);
            }
            mail.Dispose();
        });
     }
     await parallelEmails.SendingComplete();
}

In the code below, I am particularly wondering about the RunWorker method, the do while loop, and if workList.TryGet. If there is nothing in the worklist, this thread will remain looping until CompletedAdding is called or more emails are added. Is this looping an acceptable way to wait the thread?

Being an amateur, I am also grateful for any other code improvements or style points.

public class ParallelSmtpEmails : IDisposable
{
    BlockingCollection<MailTask> _workList;
    List<Task> _tasks;
    int _maxClientCount;

    public ParallelSmtpEmails(int maxSmtpClients = 5)
    {
         //removing the boundedCapacity := maxSmtpClients from BlockingCollection instantiator - the collection can be filled up without blocking
        _workList = new BlockingCollection<MailTask>(new ConcurrentQueue<MailTask>()); 
        _tasks = new List<Task>(maxSmtpClients);
        _maxClientCount = maxSmtpClients;        
    }

    public void CompletedAdding()
    {
        _workList.CompleteAdding();
    }

    public void Send(MailMessage message)
    {
        Send(message, null);
    }

    public void Send(MailMessage message, Action<SmtpException> onComplete)
    {
        _workList.Add(new MailTask() { Message = message, OnComplete = onComplete });
        if (_tasks.Count < _maxClientCount && _workList.Count > 0)
        {
            _tasks.Add(Task.Factory.StartNew(RunWorker));
        }
    }

    public Task SendingComplete()
    {
        CompletedAdding();
        return Task.WhenAll(_tasks);
    }

    public void RunWorker()
    {
        using (var client = new SmtpClient())
        {
            do
            {
                MailTask m;
                if (_workList.TryTake(out m))
                {
                    try
                    {
                        client.Send(m.Message);
                        m.OnComplete?.Invoke(null);
                    }
                    catch (SmtpException ex)
                    {
                        m.OnComplete?.Invoke(ex);
                    }
                }
            } while (!_workList.IsCompleted);
        }
    }

    #region IDisposable
    bool _disposed;
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~ParallelSmtpEmails()
    {
        Dispose(false);
    }

    void Dispose(bool disposing)
    {
        if (_disposed)
            return;

        if (disposing)
        {
            // free other managed objects that implement
            // IDisposable only
            _workList.Dispose();
        }

        // release any unmanaged objects
        // set the object references to null

        _disposed = true;
    }
    #endregion //IDisposable

    private class MailTask
    {
        public MailMessage Message { get; set; }
        public Action<SmtpException> OnComplete { get; set; }
    }
}
\$\endgroup\$
1
\$\begingroup\$

Don't use SmptClient.Send(), use SmptClient.SendMailAsync() instead. It returns a Task..

Actually, I don't think you need this class at all.

(I'm typing in a notepad, so this is untested and just to illustrate the concept.)

var mailTasks = new List<Task>();
var client = new SmtpClient();

foreach(var participant in courseParticipants)
{
    var mail = new MailMessage
    {
        From = new MailAddress("from@example.com"),
        To = participant.email,
        Subject = "come and attend course",
        BodyFormat = MailFormat.Html
    }

    string token = await GetRegistrationTokenAsync(cp);
    mail.Body = $"<div><a href='example.com/register?userToken='{token}'>let me come</a></div>";

    mailTasks.Add(client.SendMailAsync(mail));
}

try
{
    await Task.WhenAll(mailTasks);
}
catch(AggregateException ex)
{
    //handle failures
}

This code is much simpler to understand, and therefore maintain.


One last thing before I go: Are you absolutely certain that GetRegistrationTokenAsync() will always return a url safe string?

string token = await GetRegistrationTokenAsync(cp);
mail.Body = $"<div><a href='example.com/register?userToken='{token}'>let me come</a></div>";

You should probably be Url encoding the token.

\$\endgroup\$

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.