Using multithreading to send multiple emails

Question

I'm trying to send any number of emails in the shortest amount of time using .NET 5.0?

I've been playing with something like the following but I am not sure if it is optimal or even correct as there are a number of elements I don't understand.

public async Task SendEmailAsync(string subject, string htmlMessage,
    IEnumerable<string> recipients, string? attachment)
{
    using SemaphoreSlim semaphore = new(10, 10);
    await Task.WhenAll(recipients.Select(async recipient =>
    {
        await semaphore.WaitAsync();

        try
        {
            return SendEmailAsync(subject, htmlMessage, recipient, attachment);
        }
        finally
        {
            semaphore.Release();
        }
    }));
}

Can someone clarify if this is correct, or let me know if they know a better approach?

Answer 1

Nope, this code doesn't respect the semaphore. It just send all emails without concurrency degree limiting by semaphore. Because return SendEmailAsync returns right after Task object is received not the method finished, then semaphore is released immediately. Thus semaphore thottles only requests creation which I assume is fast.

The fix is await in try clause.

public async Task SendEmailAsync(string subject, string htmlMessage,
    IEnumerable<string> recipients, string? attachment)
{
    using SemaphoreSlim semaphore = new(10);
    await Task.WhenAll(recipients.Select(recipient =>
        SendEmailAsync(subject, htmlMessage, recipient, attachment, semaphore)));
}

private async Task SendEmailAsync(string subject, string htmlMessage,
    string recipient, string? attachment, SemaphoreSlim semaphore)
{
    await semaphore.WaitAsync();
    try
    {
        await SendEmailAsync(subject, htmlMessage, recipient, attachment);
    }
    finally
    {
        semaphore.Release();
    }
}

The rest part of the code looks fine for me. Probably I can only suggest to use OOP to encapsulate data for bulk emailing into some class. It would make furure code improvements easier.

Answer 2

Your approach can result in a huge number of tasks active at any given time, each of them representing load on the machine's IO resources. You need a way to limit the number of tasks.

Your approach also abuses the Select() call pretty badly. If nothing else, it makes the code very hard to read.

Here is a demonstration using a fixed number of tasks to simulate sending an email to 1000 recipients. Note that access to the shared Queue<> does not need to be synchronized in this example, but it may be needed depending on the API call used in practice, so I added synchronization. A simple lock {} suffices.

private static readonly Random random = new Random();
private static readonly Queue<string> recipients = new Queue<string>();

protected override async Task Run()
{
    for  (int i = 1; i <= 1000; ++i)
    {
        recipients.Enqueue($"recipient_{i:00000}@emaildomain.com");
    }

    List<Task> tasks = new List<Task>();

    for (int i = 1; i <= 50; ++i)
    {
        tasks.Add(SendEmails($"Task {i:00000}"));
    }

    await Task.WhenAll(tasks);
}

private static async Task SendEmails(string taskName)
{
    for (; ;)
    {
        string recipient;

        lock (recipients)
        {
            if (recipients.Count == 0)
            {
                break;
            }

            recipient = recipients.Dequeue();
        }

        Debug.WriteLine($"{taskName}: Sending to {recipient}...");
        await SendEmailAsync(recipient);
        Debug.WriteLine($"{taskName}: Sending to {recipient} complete");
    }

    Debug.WriteLine($"{taskName}: No more recipients; quitting");
}

private static async Task SendEmailAsync(string recipient)
{
    // Simulate sending an email with random network latency.
    await Task.Delay(random.Next(100, 2000));
}