0
\$\begingroup\$

In my Outlook spam add-on when I receive a new email I create a new task to process the email.

That way I don't block Outlook itself.

To prevent the List collection from growing exponentially, (traditionally Outlook is not closed for a long, long time), I have created a simple TasksController class that will help me keeping the list to a manageable size.

Then I can simply call:

TasksController.Add( ProcessNewEmail() ) 
... 
private async Task<bool> ProcessNewEmail()
{
   /// ... do stuff
}

This is the class itself

internal class TasksController : IDisposable
{
  private static TasksController _controller;

  private static TasksController Controller => _controller ?? (_controller = new TasksController());

  private readonly object _lock = new object();
  private readonly List<Task> _tasks;

  private TasksController()
  {
    _tasks = new List<Task>();
  }

  public void Dispose()
  {
    WaitAll();
  }

  private void _Add( Task run )
  {
    //  remove all the completed tasks
    RemoveAllCompleted();

    // and add the new task
    lock (_lock)
    {
      _tasks.Add(run);
    }
  }

  private void _WaitAll()
  {
    lock (_lock)
    {
      Task.WaitAll(_tasks.ToArray());
    }
    RemoveAllCompleted();
  }

  private void RemoveAllCompleted()
  {
    lock(_lock)
    {
      _tasks.RemoveAll(t => t.IsCompleted);
    }
  }

  public static void WaitAll()
  {
    Controller._WaitAll();
  }

  public static void Add(Task run)
  {
    Controller._Add( run );
  }
}

Is deriving from IDisposable necessary? Is the class itself thread safe?

\$\endgroup\$
2
\$\begingroup\$

Is the class itself thread safe

Looks fairly safe except TasksController Controller part.

Firstly, I would suggest to use .NET classes for storing concurent data instead of List<T> depending on your needs you can use ConcurrentBag<T>, ConcurrentQueue<T>. Secondly, create lock for TasksController Controller and use it inside getter. Something like this:

private static TasksController Controller {
    get{
        lock(_controllerLock){ 
           if(_controller == null){ (_controller = new TasksController();} 
           return _controller
        }
    }}

Is deriving from IDisposable necessary

I would say: no. All tasks should be disposed automatically when they finish.

However, there is only one but, you might to cancell your tasks when object is being disposed. That means if they do some heavy calculations which you do not need anymore after main object disposes. See this answer for information how to do that.

For more information about disposing tasks see MSDN blog post

\$\endgroup\$
  • \$\begingroup\$ Ok, I will make _controller volatile and do a double lock check, might be an overkill to double check, but it would prevent unneeded locks. I agree with IDisposable, but I am worried that the list will just be cleared and we will not wait for Tasks to complete, and as you said, I might want to cancel the tasks. \$\endgroup\$ – FFMG Oct 30 '17 at 6:28
  • \$\begingroup\$ ConcurrentBag<> are nice, except when removing items. Not sure what is more efficient a lock or cloning to remove completed tasks. \$\endgroup\$ – FFMG Oct 30 '17 at 6:49
  • \$\begingroup\$ This is not a double check if you want to make TasksController fully thread safe. I don't know how your ` ProcessNewEmail()` method is efficent but I if it takes some time then you can safely ignore overhead of ConcurentBag<T>. Another thinkg that came into my mind today, you can always use thread pool which can wrap up your tasks nicely. \$\endgroup\$ – MaLiN2223 Oct 30 '17 at 16:17

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.