13

I am trying to implement a simple logging library which will be used across multiple projects. The job of library is to send HTTP requests to ElasticSearch. The main point of this library is that it must not wait for the response. Also, I don't care about any error/exceptions. It must send the request to ElasticSearch, and immediately return. I don't want to make interfaces with return type Task, I want them to stay void.

Below is my sample code. Is it a correct and safe implementation of "Fire and Forget"? Is it ok if I use Task.Run() in a high load library? Or should I avoid using Task.Run() in my case? Also, if I don't use await with Task.Run(), will I block the thread? This code is in the library:

public enum LogLevel
{
    Trace = 1,
    Debug = 2,
    Info = 3,
    Warn = 4,
    Error = 5,
    Fatal = 6
}

public interface ILogger
{
    void Info(string action, string message);
}

public class Logger : ILogger
{
    private static readonly HttpClient _httpClient = new HttpClient(new HttpClientHandler { Proxy = null, UseProxy = false });
    private static IConfigurationRoot _configuration;

    public Logger(IConfigurationRoot configuration)
    {
        _configuration = configuration;
    }

    public void Info(string action, string message)
    {
        Task.Run(() => Post(action, message, LogLevel.Info));
        /*Post(action, message, LogLevel.Info);*/ // Or should I just use it like this?
    }

    private async Task Post(string action, string message, LogLevel logLevel)
    {
        // Here I have some logic

        var jsonData = JsonConvert.SerializeObject(log);
        var content = new StringContent(jsonData, Encoding.UTF8, "application/json");

        var response = await _httpClient.PostAsync(_configuration.GetValue<string>("ElasticLogger:Url"), content);
        // No work here, the end of the method
    }
}

This is how I register logger inside ConfigureServices method in Startup class of my web api:

public void ConfigureServices(IServiceCollection services)
{
     // ......

     services.AddSingleton<ILogger, Logger>();

     // .....
}

This code is in a method inside my web api:

public void ExecuteOperation(ExecOperationRequest request)
{
    // Here some business logic

    _logger.Info("ExecuteOperation", "START"); // Log

   // Here also some business logic

    _logger.Info("ExecuteOperation", "END"); // Log
}
Nomad
  • 313
  • 2
  • 4
  • 18
  • 2
    If possible, I'd consider to take a look at Serilog and the Elastic Sink, it might save you a lot of trouble: https://github.com/serilog/serilog-sinks-elasticsearch – Wiebe Tijsma Jun 08 '18 at 08:54
  • "I don't care about any error/exceptions" - then surely the simplest implementation is one that assumes that every request will error and thus does nothing? – Damien_The_Unbeliever Jun 08 '18 at 09:10
  • @Damien_The_Unbeliever What I wanted to emphasize on was not on error/exception handling, but rather on the correct/proper implementation of fire and forget method given some constraints. – Nomad Jun 08 '18 at 09:14
  • 1
    Possible duplicate of [Web Api - Fire and Forget](https://stackoverflow.com/questions/36335345/web-api-fire-and-forget) – Shaun Luttin Jun 09 '18 at 02:14
  • Possible duplicate of https://stackoverflow.com/questions/18502745/fire-and-forget-async-method-in-asp-net-mvc – Shaun Luttin Jun 09 '18 at 02:16
  • Possible duplicate of [Fire and forget async method in asp.net mvc](https://stackoverflow.com/questions/18502745/fire-and-forget-async-method-in-asp-net-mvc) – Michael Freidgeim Nov 02 '18 at 21:22

1 Answers1

8

Re : Unawaited call to an async method vs Task.Run()

Since there's only a small amount of CPU bound work in Post (i.e. creating json payload), there's no benefit of another Task.Run - the overhead of scheduling a new Task on the Threadpool will outweigh any benefit IMO. i.e.

Post(action, message, LogLevel.Info);*/ // Or should I just use it like this?

is the better of the two approaches. You'll likely want to suppress the compiler warning associated within unawaited Tasks and leave a comment for the next dev to come across the code.

But as per Stephen Cleary's definitive answer, fire and forget in ASP.Net is almost never a good idea. Preferable would be to offload work, e.g. via a queue, to a Windows Service, Azure Web Job et al.

There are additional dangers - if the unawaited Task throws, you'll want to observe the exception.

Also, note that any work done after the Post (e.g. if you work with response) that this is still a continuation Task which needs to be scheduled on the Threadpool - if you fire off high volumes of your Post method, you'll wind up with a lot of thread contention when they complete.

Re : Also, if I don't use await with Task.Run(), will I block thread?

await doesn't require a thread. await is syntactic sugar to ask the compiler to rewrite your code asynchronously. Task.Run() will schedule a second task on the ThreadPool, which will only do a tiny amount of work before it hits the PostAsync method, which is why the recommendation is not to use it.

The amount of caller thread usage/block on the unawaited call from Info to Post depends on what kind of work is done before the Task is returned. In your case the Json serialization work would be done on the caller's thread (I've labelled #1), however the execution time should be negligible in comparison to the HTTP call duration. So although not awaited by method Info, any code after the HTTP call will still need to be scheduled when the Http call completes, and will be scheduled on any available thread (#2).

public void Info(string action, string message)
{
#pragma warning disable 4014 // Deliberate fire and forget
    Post(action, message, LogLevel.Info); // Unawaited Task, thread #1
#pragma warning restore 4014
}

private async Task Post(string action, string message, LogLevel logLevel)
{
    var jsonData = JsonConvert.SerializeObject(log); // #1
    var content = new StringContent(jsonData, Encoding.UTF8, "application/json"); // #1

    var response = await httpClient.PostAsync(...), content);

    // Work here will be scheduled on any available Thread, after PostAsync completes #2
}

Re: Exception Handling

try..catch blocks work with async code - await will check for a faulted Task and raise an exception:

 public async Task Post()
 {
     try
     {
         // ... other serialization code here ...
         await HttpPostAsync();
     }
     catch (Exception ex)
     {
         // Do you have a logger of last resort?
         Trace.WriteLine(ex.Message);
     }
 }

Although the above will meet the criteria for observing the exception, it is still a good idea to register an UnobservedTaskException handler at the global level.

This will help you detect and identify where you've failed to observe an exception:

TaskScheduler.UnobservedTaskException += (sender, eventArgs) =>
{
    eventArgs.SetObserved();
    ((AggregateException)eventArgs.Exception).Handle(ex =>
    {
        // Arriving here is BAD - means we've forgotten an exception handler around await
        // Or haven't checked for `.IsFaulted` on `.ContinueWith`
        Trace.WriteLine($"Unobserved Exception {ex.Message}");
        return true;
    });
};

Note that the above handler is only triggered when the Task is collected by the GC, which can be some time after the occurrence of the exception.

Michael Freidgeim
  • 26,542
  • 16
  • 152
  • 170
StuartLC
  • 104,537
  • 17
  • 209
  • 285
  • The problem is less the observing of the exception. The problem is if an exception happens with it, it most-likely ends up crashing your application unless there's an application wide catcher. Also when the request ends, the transient/scoped services will get disposed and cant be used there. The queue with a background service is definitely the right way of doing that – Tseng Jun 08 '18 at 08:51
  • [There was a change](https://stackoverflow.com/q/16093846/314291) whereby `UnobservedTaskException`s should no longer crash the process, but yes, F+F isn't the way forward here. – StuartLC Jun 08 '18 at 08:56
  • @StuartLC Thank you for reply. What if I don't care about exceptions that are thrown by Task? Also, I added some more code to a question to make it more clear. – Nomad Jun 08 '18 at 09:05
  • The linked answer states that behavio for unobserved **Task**. `async void` is different and **shouldn't ever** be used outside of UI event callbacks (WPF, Windows Forms). @Nomad: This won't change anything on the topic of your services potentially getting disposed before your operation is complete – Tseng Jun 08 '18 at 09:08
  • @Tseng Do you mean that in .NET Core UnobservedTaskException still crashes process? So if I add my logger as singleton service, and there will be UnobservedTaskException in library, my application(web api) will crash? – Nomad Jun 09 '18 at 07:15
  • @Nomad I've updated with Exception handling (i.e. wrap in try-catch as per usual). [UnobservedTaskException](https://stackoverflow.com/q/43639601/314291) handler is available in all recent versions of core, but just remember, it is only fired when GC collects faulted tasks – StuartLC Jun 09 '18 at 07:24
  • @StuartLC Just wrapping inside try-catch will be sufficient? Or should I go with your second suggestion to use TaskScheduler.UnobservedTaskException? – Nomad Jun 09 '18 at 07:38
  • Use both. `try..catch` will catch + observe the exception (and since this is the logger which is failing, and given that the caller has long since disappeared, you don't really have much recourse for actually handling the exception, hence `Trace`). UnobservedTaskException will highlight places where you've triggered F+F but haven't observed the exception. During debug sessions, place a break point there, or better yet, log to disk. – StuartLC Jun 09 '18 at 08:02
  • @StuartLC Sorry, for this stupid question, but should I place the code with TaskScheduler.UnobservedTaskException += ... inside catch(Exception ex) {} block? – Nomad Jun 09 '18 at 10:57
  • No - this needs to be registered at global level - best place is likely the [`Startup.Cs`](https://learn.microsoft.com/en-us/aspnet/core/fundamentals/startup?view=aspnetcore-2.1) – StuartLC Jun 09 '18 at 11:08