5

I have a .NET CORE 2 backend. In one of my controller endpoints, I'm creating invitations to be sent out via email. This seems to be a huge bottleneck on the endpoint and after thinking about it, I don't really need to wait for these invitations. If the email fails to send out, I can't really do anything about it anyway.

If I don't do await sendFn() would it essentially be a fire and forget method? I was reading on another stackoverflow thread that I'd have to do sendFn().ContinueWith(t => throw(t)) to be able to catch the exception since it'll be in another thread.

I have similar mailing functions around the code base. They each do slightly different things, but is there a service fn I can do to wrap these to make them fire and forget? I think some places I can just not use await (if that works), but some things alter the database context so if I don't await them I can potentially run into a case where something is accessing the same db context.

[HttpPost]
public async Task<IActionResult> CreateEvent([FromBody] Event val)
{
    _ctx.Event.Add(val);
    await _ctx.SaveChangesAsync();

    await SendInvitations(val); // fn in question

    return Ok();
}

public async Task SendInvitation(Event event)
{
   forEach (var person in event.people)
   {
     await _ctx.Invitation.Add(person); // This shouldn't happen while another iteration or some other async code elsewhere is using the db ctx.
     _ctx.SaveChangesAsync();
     await _mailService.SendMail(person.email,"you have been invited"); // don't really need to await this.

   }
}

I'm posting to my server with data about an event. After I create and save the event to the database, I go and create invitations for each person. These invitations are also database items. I then send out an email. I'm mostly worried that if I drop the await, then when I'm creating invitations, it may conflict with db context elsewhere or the next iteration.

user6728767
  • 1,123
  • 3
  • 15
  • 31
  • 2
    It would really be super if you could provide a [mcve]. – Enigmativity Jul 20 '18 at 00:35
  • I added a very simplified version of my code. – user6728767 Jul 20 '18 at 00:46
  • @user6728767 - It'd be great if you added working code. Currently it doesn't compile. Did you read [mcve]? – Enigmativity Jul 20 '18 at 00:49
  • Fire and forget is bad = if you don't care that the code completes successfully, then why have it run in the first place? More likely you should queue this work to run in the background. Perhaps using a background worker service (maybe using Hangfire or processing messages off a service bus) – mason Jul 20 '18 at 00:49
  • 1
    @mason - It seems to me that fire and forget is perfectly fine. UDP on the internet is a protocol based solely on fire and forget. The DNS system works using it. – Enigmativity Jul 20 '18 at 00:52
  • UDP is used for things where it's *okay* to drop data. Fault tolerance. If this email fails to send, that is probably *not okay*. The error needs to be captured and logged and possibly someone needs to be notified. Fire and forget at an application level is not a good idea. – mason Jul 20 '18 at 00:54
  • 1
    @mason - The OP tells us that it's ok that that don't send - "I don't really need to wait for these invitations. If the email fails to send out, I can't really do anything about it anyway." – Enigmativity Jul 20 '18 at 00:58
  • @Enigmativity Then the OP is mistaken. They could be using the wrong username, password, host etc. All things that you can find out about by catching errors and logging them. If the OP truly doesn't care if the invitations don't get sent, they shouldn't write the code in the first place. – mason Jul 20 '18 at 01:02
  • The email is sent out just to notify the user via email. It'd be helpful to have, but at the same time it's not going to kill the app if it doesn't send. The user can still login and check their dashboard. – user6728767 Jul 20 '18 at 01:04
  • Let's say some change happens where every single email fails to go out. Maybe the mail server host changes, or the password used to send the emails is expired. Now all your emails are going to fail to go out. And if you do it fire and forget style, you're not going to have any idea that something is wrong until a user comes to you and tells you. And even then, you might not know that *all* the emails failed. And that it's been down for weeks but no one has said anything. That's why fire and forget makes no sense. Have it be asynchronous, but handle the errors. So Hangfire or service bus etc – mason Jul 20 '18 at 01:09
  • 1
    @mason - You are right, these are all good things. However, the OP says it doesn't matter so it's noise, and not signal, in answering this question. – Enigmativity Jul 20 '18 at 01:17

2 Answers2

5

To get your code to compile and run I had to make these changes:

public async Task<IActionResult> CreateEvent(Event val)
{
    _ctx.Event.Add(val);
    await _ctx.SaveChangesAsync();
    await SendInvitation(val);
    return Ok();
}

public async Task SendInvitation(Event @event)
{
    foreach (var person in @event.people)
    {
        await _ctx.Invitation.Add(person);
        await _ctx.SaveChangesAsync();
        await _mailService.SendMail(person.email, "you have been invited");
    }
}

I also had to write this harness code:

public OK Ok() => new OK();

public class Event
{
    public List<Person> people = new List<Person>();
}

public class Person
{
    public string email;
}

public interface IActionResult { }

public class OK : IActionResult { }

public class Invitation
{
    public Task Add(Person person) => Task.Run(() => { });
}

public static class _ctx
{
    public static List<Event> Event = new List<Event>();
    public static Invitation Invitation = new Invitation();
    public static Task SaveChangesAsync() { return Task.Run(() => { }); }
}

public static class _mailService
{
    public static Task SendMail(string email, string message) { return Task.Run(() => { }); }
}

Then I updated SendInvitation like this:

public async Task SendInvitation(Event @event)
{
    Thread.Sleep(2000);
    foreach (var person in @event.people)
    {
        await _ctx.Invitation.Add(person);
        await _ctx.SaveChangesAsync();
        await _mailService.SendMail(person.email, "you have been invited");
    }
    Console.WriteLine("Done `SendInvitation`.");
}

Now, I can run it all like so:

var e = new Event();
e.people.Add(new Person() { email = "foo@bar.com" });
CreateEvent(e).ContinueWith(t => Console.WriteLine("Done `CreateEvent`."));
Console.WriteLine("Done `Main`.");

That outputs:

Done `Main`.

Then 2 seconds later:

Done `SendInvitation`.
Done `CreateEvent`.

If I simply change CreateEvent to this:

public async Task<IActionResult> CreateEvent(Event val)
{
    _ctx.Event.Add(val);
    await _ctx.SaveChangesAsync();
    Task.Run(() => SendInvitation(val));
    return Ok();
}

Then I get this output:

Done `Main`.
Done `CreateEvent`.

Then 2 seconds later:

Done `SendInvitation`.

That seems to be what you want.

Enigmativity
  • 113,464
  • 11
  • 89
  • 172
  • thanks for the detailed answer. I apologize for not including the harness code. It totally slipped my mind and for some reason I thought those .NET stuff would have compiled. Thanks for taking the time in writing that. When you put 'If I simply change CreateEvent to this', what was it before? Also, continueWith just makes sure the given lambda is ran when the task is done right, but in the original context? – user6728767 Jul 23 '18 at 17:22
  • @user6728767 - Both versions of `CreateEvent` are in my answer. The `ContinueWith` just runs the continuation task after the main one completes - I'm not sure about the context (do you mean thread?). – Enigmativity Jul 23 '18 at 23:57
  • So in my case would I just be doing: `CreateEvent.ContinueWith(e => // handle exception, , TaskContinuationOptions.OnlyOnFaulted)`. This would run the CreateEvent function as `Fire and Forget`, but only call the continuation if some exception occurs right? – user6728767 Jul 24 '18 at 00:06
  • @user6728767 - That will cancel the original task if I understand the continuation option correctly. Try `Task.Run(() => 42).ContinueWith(e => -1, TaskContinuationOptions.OnlyOnFaulted);` - it throws. – Enigmativity Jul 24 '18 at 00:29
3

The short answer is that you have no guarantees that that the execution of that code will complete.

That's why ASP.NET Core has infrastructure for background work: Implementing background tasks in .NET Core 2.x webapps or microservices with IHostedService and the BackgroundService class

Paulo Morgado
  • 14,111
  • 3
  • 31
  • 59
  • 4
    Do you have a reference to the docs or somewhere else that specifically states that a non-awaited task is not guaranteed to complete? My question is precisely that - if I don't await within an ASP.NET Core controller, is there a chance that my controller will be torn down before the task completes? I get that there's infrastructure for background services, but that's overkill for simple tasks. – Brian S Oct 17 '18 at 19:15
  • Do you have a reference to the docs or somewhere else that specifically states that a non-awaited task is guaranteed to complete? – Paulo Morgado Oct 17 '18 at 19:37
  • 2
    Thanks for responding, Paulo. No, I don't have a reference that states that it IS guaranteed to complete, but neither do I have a references that says it IS NOT guaranteed to complete. That's why I'm asking. I assume it's the latter (not guaranteed), however I'd love to confirm my assumption via some reliable source. – Brian S Oct 30 '18 at 14:18