2
\$\begingroup\$

This class keeps track of tasks which are added and tries to remove them after certain delay. I tried doing it with CustomTaskScheduler but didn't get much in return.

Here is my related question on SO.

I would be glad to know if there are other ways to track tasks.

public class TaskCompletedNotifier
{
    public delegate void AllTasksCompletedHandler(object sender);
    public delegate void TaskStartedHandler();
    public event AllTasksCompletedHandler AllTasksCompleted;
    public event TaskStartedHandler TaskStarted;

    private object syncLock = new object();
    private SynchronizedCollection<Task> tasksCollection;
    private bool isTaskStartedNotified;
    private readonly uint delay;

    public TaskCompletedNotifier(uint delayBeforeRemovingTasks)
    {
        tasksCollection = new SynchronizedCollection<Task>();
        delay = delayBeforeRemovingTasks;
    }

    public void Add(Task task)
    {

        if (!isTaskStartedNotified)
        {
            isTaskStartedNotified = true;
            TaskStarted?.Invoke();
        }

        task.ContinueWith(t =>
        {
            RemoveTask(t);
        });

        tasksCollection.Add(task);
    }


    private async void RemoveTask(Task task)
    {
        await Task.Delay(300);
        await Task.Run(() =>
        {
            tasksCollection.Remove(task);

            if (tasksCollection.Count == 0)
            {
                isTaskStartedNotified = false;
                AllTasksCompleted?.Invoke(tasksCollection);
            }
        });
    }
}
\$\endgroup\$
1
\$\begingroup\$

I don't think you want a TaskScheduler and this is a better idea. Creating a custom TaskScheduler isn't for the faint of heart and the ThreadPoolTaskScheduler does a lot of things and it's better to just let it do it's own thing.

Your code is headed down the correct path but I'd make a couple of changes. One for the delay I wouldn't use a uint I'd use a TimeSpan. That easier to understand and can use Factory method like TimeSpan.FromMilliseconds to indicate what is the delay. I don't like passing in numbers. Is it milliseconds? seconds? Also from your code you are not using the delay.

Also why create delegates for the events and not just use the standard EventHandler? Also when sending out the source of the event you are sending our your collection. This seems like a very bad idea. What if someone would clear your collection or add to it outside your class?

You have a synclock that you are not using. Using the SynchronizedCollection wouldn't require a lock. But that being said I would probably ditch the SyncronizedCollection. That will do a lot more locking than what is required for this application. I would just use a HashSet and and lock on that since it's going to be internal.

This now becomes the class and the constructor

public class TaskCompletedNotifier
{
    public event EventHandler AllTasksCompleted;
    public event EventHandler TaskStarted;
    private readonly ISet<Task> _tasksCollection = new HashSet<Task>();
    private readonly TimeSpan _delay;

    public TaskCompletedNotifier(TimeSpan delayBeforeRemovingTasks)
    {
        if (delayBeforeRemovingTasks.TotalMilliseconds < 1)
        {
            throw new ArgumentOutOfRangeException(nameof(delayBeforeRemovingTasks));
        }

        _delay = delayBeforeRemovingTasks;
    }

Now when adding a task to the collection we just need to account for the delay and we can do that with the Task.Delay and combo that up with Task.WhenAny. Also any time there is an async void that is a code smell. I would just run the remove from the ContinueWith and not get task involved. I wouldn't think the overhead of the thread switching would be that great for just removing an item from a hashset.

public void Add(Task task)
{
    // short-cut why add a task that is already done?
    if (!task.IsCompleted)
    {
        // Create a task that is done when either the time has passed or when it's completed
        var newTask = Task.WhenAny(task, Task.Delay(_delay));
        var fireEvent = false;
        lock (_tasksCollection)
        {
            if (_tasksCollection.Add(newTask))
            {
                // set if we need to fire event
                fireEvent = (_tasksCollection.Count == 1);
            }
        }

        // fire event outside of locker
        if (fireEvent)
        {
            // Don't pass in the internal collection as we don't want anyone modifying our data outside our self
            TaskStarted?.Invoke(this, EventArgs.Empty);
        }

        // add the continuation to remove it from the collection
        //     after we might have fired the event to ensure the events run in correct order
        newTask.ContinueWith(RemoveTask);
    }
}

private void RemoveTask(Task task)
{
    var fireEvent = false;
    lock (_tasksCollection)
    {
        {
            if (_tasksCollection.Remove(task))
            {
                fireEvent = (_tasksCollection.Count == 0);
            }
        }
    }

    if (fireEvent)
    {
        AllTasksCompleted?.Invoke(this, EventArgs.Empty);
    }
}

It's important to note that you should still await the task that is passed into this class outside of this class to handle any faults.

\$\endgroup\$
  • \$\begingroup\$ Thanks @CharlesNRice. This is a good start. Appreciate the time and effort to make it better. \$\endgroup\$ – Sharad Shahi Oct 15 '17 at 16:43

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.