-3

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?

Jonathan Wood
  • 65,341
  • 71
  • 269
  • 466
  • @Jonathon I can help you with this, but you'll need to edit the question so it can be re-opened. – glenebob Jul 08 '21 at 20:47
  • 1
    @glenebob: I don't know how else to ask how to do this. Do I just get rid of the code I have so far? Conceptually, this question is dead simple. I don't understand the problem. – Jonathan Wood Jul 08 '21 at 20:49
  • Would this might solve your what you are asking for https://stackoverflow.com/questions/47939504/sending-5000-messages-in-async-way-in-c-sharp/47946028#47946028 – Maytham Fahmi Jul 08 '21 at 21:04
  • @maytham-ɯɐɥʇʎɐɯ: This is an older question. I was under the impression the newer constructs were preferred, but because they are newer constructs, I'm having a bit of trouble getting authoritative information on this. – Jonathan Wood Jul 08 '21 at 21:07
  • @Jonathon try asking something like "using concurrent tasks to execute an async method in a controlled fashion". This isn't really a threading problem when you can use asynchronous email services, and it isn't really about email either. Also, sending an email within a Select predicate is, uh, rather abusive. I have a working sample for you, I just need a way to get it to you. – glenebob Jul 08 '21 at 21:40
  • _"Can someone clarify if this is correct, or let me know if they know a better approach?"_ -- is not an appropriate question for Stack Overflow, as it's primarily opinion based (indeed, entirely opinion based). If the code you have above works for your scenario, and you cannot provide a clear description of some _problem_ you're unable to solve, the question is off-topic. You may consider the softwareengineering or codereview Stack Exchange sites, depending on what kind of question you're willing to post (codereview in particular has very strict requirements themselves). – Peter Duniho Jul 08 '21 at 21:46
  • Jonathon I'm on Code Review as well, if you'd like to try over there. I can drop my sample there. – glenebob Jul 08 '21 at 22:19
  • @glenebob: Done. (Although one more vote to reopen this question and people could again answer here.) – Jonathan Wood Jul 08 '21 at 22:26

1 Answers1

1

If you want to send the emails in the shortest amount of time, you should probably try first to send them all at once like this:

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

This will invoke the SendEmailAsync method for all recipients concurrently, and practically instantly. It can't get much faster than that!

Your current code that employs a SemaphoreSlim implies that your email delivery service chokes when bombarded with multiple emails concurrently, and for this reason you have limited the concurrency to 10. Your code is excellent at limiting the concurrency of the asynchronous operations (provided that you fix the missing await before the SendEmailAsync call), but whether this throttling helps at improving the throughput of your email delivery service can only be found by experimentation.

Also, as @Krythic mentioned in a comment, the email service providers may enforce non-technical limitations at how many emails they can accept for delivery, to combat the phenomenon of spam.

Btw there is no multithreading involved in your code. The SendEmailAsync is invoked multiple times sequentially on the current thread, and the asynchronous email deliveries are most likely flying on no thread at all.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • But what if there were 1,000 emails? Isn't some sort of throttling required? – Jonathan Wood Jul 08 '21 at 19:55
  • @JonathanWood you should check the specifications of your email delivery service. It may be able to handle 1,000,000 concurrent requests with ease. – Theodor Zoulias Jul 08 '21 at 20:01
  • 1
    Thanks, but your code gives me a CS1998 warning at the `=>`: *This async method lacks 'await' operators and will run synchronously. Consider using the 'await' keyword operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.* – Jonathan Wood Jul 08 '21 at 20:25
  • Should the `await` before the `recipient =>` be removed? – Jonathan Wood Jul 08 '21 at 20:31
  • 1
    This approach can result in a very unreasonable number of tasks active at once. It has nothing to do with the email service because you cannot know how many other clients are also trying to send emails. You should find an approach that limits the number of tasks. I'd guess something between 10 and 50 tasks might be reasonable. – glenebob Jul 08 '21 at 20:33
  • @glenebob: So what is your suggestion? If I simply broke up my emails into blocks of 50, that would still waste time because I wouldn't start sending email number 51 until every last email of the first block was done. – Jonathan Wood Jul 08 '21 at 20:37
  • @JonathanWood you are right, there is a bug. The task returned from the `SendEmailAsync` method should be awaited. I just fixed it. The same bug exists in your own code too (the code that uses a `SemaphoreSlim`). – Theodor Zoulias Jul 08 '21 at 21:45
  • @JonathanWood as you said the bug in my code could also be fixed by removing the `async` before the `recipient`. But [eliding Async and Await](https://blog.stephencleary.com/2016/12/eliding-async-await.html) is not advisable in general, and would be a bad idea here in particular. It would open the window for fire-and-forget tasks, in case *some* calls to the `SendEmailAsync` method failed synchronously. – Theodor Zoulias Jul 08 '21 at 21:58
  • @glenebob without knowing the specifications of the email service, any assessment about what number of concurrent tasks is *reasonable* is just wild speculation. The service could just as easily choke with 5 concurrent requests, or handle 1,000,000 concurrent requests like a boss. On the client side a pending `Task` is just an object in memory, usually consuming less than 1,000 bytes. So if the service can handle the load, the client can handle it too. – Theodor Zoulias Jul 08 '21 at 22:10
  • @TheodorZoulias that's pretty much my point in saying it has nothing to do with the email service. This is a local resource problem. – glenebob Jul 08 '21 at 22:15
  • 1
    Just wanted to add that if you programatically send too many emails at once, Gmail or whatever will straight up block you. I once wrote a program to send out mass emails, and gmail blocked it after a dozen or so emails. Not sure how the system works, but they do indeed have mechanisms in place to prevent this. – Krythic Jul 08 '21 at 23:24
  • @Krythic good point. There may be non-technical limitations at how many emails can be sent during a specific amount of time. Αnd the rules can be complex, or even unpublished, for security reasons. – Theodor Zoulias Jul 08 '21 at 23:32