Simple async task scheduler

Question

I've written a .NET Core console application to monitor the operation of a sensor network and I'd like a review of the task scheduling performed by the main program loop. There are three tasks that are all declared as public static async Task that perform the following operations:

• CheckGateways.CheckAll connects to an external API at one minute intervals and is the most likely to fail or take a long time. Normally it'll take a few seconds but I'd like other tasks to continue meanwhile.

• CheckNodes.CheckAll is an internal database check that may take a few seconds but is not time critical and runs once per minute.

• CheckAlerts.CheckAll checks for alert conditions on the sensors and is the most time critical so I'm checking that once per second.

While the code seems to be working it hasn't been well stress-tested and a few things I'd like reviewed are:

• Are there any potential race conditions with the way I'm checking the status of the first two tasks?

• I'm repeating the code to start / check the first two tasks so perhaps there's a cleaner way to do that without introduction too much extra code as there's unlikely to be any more tasks added.

• Also any general comments on coding / naming standards would be appreciated.

  static async Task Main()
  {
      IConfiguration configuration = new ConfigurationBuilder()
          .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
          .Build();
      Log.Logger = new LoggerConfiguration()
          .ReadFrom.Configuration(configuration)
          .Enrich.FromLogContext()
          .MinimumLevel.Override("Microsoft", LogEventLevel.Warning)
          .CreateLogger();
      Task checkGateways = null;
      DateTime lastGatewayCheck = DateTime.UtcNow;
      Task checkNodes = null;
      DateTime lastNodeCheck = DateTime.UtcNow;
      while (true)
      {
          try
          {
              // Check gateway connectivity loss at one minute intervals
              if (DateTime.UtcNow.Subtract(lastGatewayCheck).TotalMinutes >= 1
                      && (checkGateways == null || checkGateways.IsCompleted))
              {
                  lastGatewayCheck = DateTime.UtcNow;
                  checkGateways = CheckGateways.CheckAll(configuration);
              }
              if (checkGateways?.Status == TaskStatus.Faulted)
              {
                  throw checkGateways.Exception;
              }
  
              // Check node connectivity loss at one minute intervals
              if (DateTime.UtcNow.Subtract(lastNodeCheck).TotalMinutes >= 1
                      && (checkNodes == null || checkNodes.IsCompleted))
              {
                  lastNodeCheck = DateTime.UtcNow;
                  checkNodes = CheckNodes.CheckAll(configuration);
              }
              if (checkNodes?.Status == TaskStatus.Faulted)
              {
                  throw checkNodes.Exception;
              }
  
              // Check for pending alerts to send at one second interval
              await CheckAlerts.CheckAll(configuration);
          }
          catch (AggregateException ae)
          {
              foreach (var ex in ae.InnerExceptions)
              {
                  LogException(ex);
              }
          }
          catch (Exception ex)
          {
              LogException(ex);
          }
          await Task.Delay(1000);
      }
  }
  
  public static void LogException(Exception ex)
  {
      ConsoleMessage(ex.ToString());
      Serilog.Log.Error(ex, "Error occured in TelemetryService");
  }
  
  public static void ConsoleMessage(string msg)
  {
      Console.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss}: {msg}");
  }
  

Answer 1

No way to stop the cycle.

The standard with TPL is to use the CancellationToken. Even if it is not need now, which I don't know why, it would be easier in the future if it supported it. For example if it gets turned into a service can just cancel the Token Source or if stays a console app could trap Esc key and cancel token source.

Numbers for Task Delay.

It's easier to read/maintain

Task.Delay(TimeSpan.FromSeconds(1))

then

Task.Delay(1000)

For each Task instead of checking their time you can combine them with Task.Delay()

For example

Task.WhenAll(lastNodeCheck, Task.Delay(TimeSpan.FromMinutes(1)));

Now we have one task that will only complete when either a minute has passed and the main task has completed. We can make a helper method for this

    public static Task DelayedTask(Task task, TimeSpan delay, CancellationToken token)
    {
        return Task.WhenAll(task, Task.Delay(delay, token));
    }

The downside to this is even if the task failed right away it will still wait the delay time before it gets logged that it failed. I don't think that's a big deal breaker but only you know that for sure.

Optional but you can make this into a queue

We can create a dictionary that waits for task to be complete then re-adds them if needed. This will make the code a bit more complex but easier to add new task later on.

Something like

    private static async Task TaskQueue(CancellationToken token, params Func<Task>[] tasks)
    {
        if (tasks.Length == 0)
        {
            return;
        }
        
        var queue = new ConcurrentDictionary<Task, Func<Task>>();
        foreach (var task in tasks)
        {
            queue.TryAdd(task(), task);
        }

        while (!token.IsCancellationRequested)
        {
            await Task.WhenAny(queue.Keys).ContinueWith(completedTask =>
            {
                Func<Task> factory;
                var mainTask = completedTask.Unwrap();
                queue.TryRemove(mainTask, out factory);
                if (!mainTask.IsCanceled)
                {
                    queue.GetOrAdd(factory(), factory);
                }

                if (mainTask.IsFaulted)
                {
                    foreach (var ex in mainTask.Exception.InnerExceptions)
                    {
                        LogException(ex);
                    }
                }
            });
        }
    }

I haven't tested this with all options but with basic test seems to work. We create a concurrent dictionary and load it with tasks that are in flight and factories to make the task. As task complete we remove from the dictionary and re-add them. Continue until we get told to cancel.

Now in the main method can look something similar to this

        var cancellation = new CancellationTokenSource();
        var cancelToken = cancellation.Token;
        Func<Task> checkGateWayFactory = () => DelayedTask(CheckGateways.CheckAll(configuration), TimeSpan.FromMinutes(1), cancelToken);
        Func<Task> checkNodeFactory = () => DelayedTask(CheckNodes.CheckAll(configuration), TimeSpan.FromMinutes(1), cancelToken);
        Func<Task> checkAlertFactory = () => DelayedTask(CheckAlerts.CheckAll(configuration), TimeSpan.FromSeconds(1), cancelToken);
        await TaskQueue(cancelToken, 
            checkAlertFactory, 
            checkGateWayFactory, 
            checkNodeFactory);

Answer 2

• Keep a single var now = DateTime.UtcNow;
• Initialize the check-tasks to Task.CompletedTask to get rid of all the null checks.
• Check if the tasks are faulted, and use Task.IsFaulted, before rescheduling the task to avoid swallowing last second exceptions.
• If one of the 1-minute tasks fails early the rest of the checks won't run until the task is rescheduled.
• Use timers instead of loops over tasks.