1
\$\begingroup\$

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?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ To the best of your knowledge, does the code work as intended? If not, then the post is off-topic - note that the help page states "If you are looking for feedback on a specific working piece of code...then you are in the right place!" \$\endgroup\$ Jul 8 at 22:40
  • \$\begingroup\$ What makes you think the email client can handle parallel requests? I’d assume they are all just queued up and processed sequentially there anyway. \$\endgroup\$
    – Aganju
    Jul 8 at 23:55
  • \$\begingroup\$ @Aganju: Most of the time it takes to send an email is spent waiting for the server to respond. It would be stupid to queue them all up. At any rate, I'm not responsible for if mail server designers make stupid designs. I'm just responsible that my code doesn't have a stupid design. \$\endgroup\$ Jul 9 at 1:18
3
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ Would this not be the same as adding the await keyword before calling SendEmailAsync() in my code? \$\endgroup\$ Jul 8 at 23:02
  • \$\begingroup\$ @JonathanWood probably but you can't return void in Select, right? I'm not strong in lambdas syntax without IDE, just wrote the answer from mobile. Anyway, the method-like approach isn't slower than lambda in performance. \$\endgroup\$
    – aepot
    Jul 8 at 23:08
  • \$\begingroup\$ @JonathanWood try changing return to await. If it will compile successful then it's done. \$\endgroup\$
    – aepot
    Jul 8 at 23:14
  • 1
    \$\begingroup\$ Yes, that seems to work. A lambda doesn't necessarily return anything. \$\endgroup\$ Jul 8 at 23:17
2
\$\begingroup\$

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));
}
\$\endgroup\$
8
  • \$\begingroup\$ Queue<string> isn't thread-safe and might be damaged if async calls performed not on single-threaded SynchronizationContext. everything happens on one thread I'm not sure if that's true. \$\endgroup\$
    – aepot
    Jul 8 at 23:01
  • \$\begingroup\$ @aepot good point. Everything happens in one thread in this example, but you are correct that continuations may be scheduled on different threads depending on how the API was implemented. I'll edit to add a lock. \$\endgroup\$
    – glenebob
    Jul 8 at 23:08
  • \$\begingroup\$ ConcurrentQueue may be faster than lock. Also you may update the text. \$\endgroup\$
    – aepot
    Jul 8 at 23:16
  • 1
    \$\begingroup\$ @aepot it probly doesn't matter. The queue is just there to support the task concurrency example I threw together. OP may not go that route. The important part is how to limit the number pf tasks without using a SemaphoreSlim. \$\endgroup\$
    – glenebob
    Jul 8 at 23:20
  • 1
    \$\begingroup\$ Ok, but I'm not sure that avoiding the semaphore makes the code more efficient. Anyway I like the solution. Will make some benchmarks to ensure. \$\endgroup\$
    – aepot
    Jul 8 at 23:30

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.