1

Let's say I have the following code:

class Test
{
    Dictionary<int, Task> tasks;

    void Run()
    {
        tasks.Add(1, DoStuff(1));
        tasks.Add(2, DoStuff(2));
        tasks.Add(3, DoStuff(3));
    }

    async Task DoStuff(int id)
    {
        try
        {
            // Do stuff that takes a lot of time and can end unsuccessfully.
        }
        finally
        {
            tasks.Remove(id);
        }
    }
}

If I know for sure that task can remove only itself and all ids will be different (newly generated guid for instance) - can I leave this code as is? Or do I need some kind of locking or using ConcurrentDictionary here because internals of regular Dictionary or List can fail when accessed concurrently? (let's say there is 10000 tasks like this and they all end at the same time)

Also: nobody will ever read this dictionary/list or iterate through it. It is used solely for the purpose of keeping references to the background tasks (so GC doesn't collect them). If you have a better proposal for that use case please suggestions are welcome :)

Improve this question
3
  • You don't need to lock to remove...from the documentation: "If the Dictionary<TKey,TValue> does not contain an element with the specified key, the Dictionary<TKey,TValue> remains unchanged. No exception is thrown." It doesn't mean that it's thread-safe, however . Commented Apr 6, 2020 at 19:42
  • ".NET Core 3.0+ only: this mutating method may be safely called without invalidating active enumerators on the Dictionary<TKey,TValue> instance. This does not imply thread safety." Read that last line... Commented Apr 6, 2020 at 19:44
  • It's a typo, thanks. I'll fix Commented Apr 6, 2020 at 19:46

2 Answers 2

Reset to default
1

You definitely need some sort of locking mechanism here. Personally, I'd use ConcurrentDictionary for this. Or, depending on what you are trying to accomplish here, you could just do this:

var list = new List<Task>();

list.Add(DoStuff(1));
list.Add(DoStuff(2));
list.Add(DoStuff(3));

await Task.WhenAll(list);

This will ensure all tasks are finished before you continue.

***Edit - Update

If you want to have them continuously removed from the list - depending on how you are viewing that, you can just do this instead:

private List<Task> _list = new List<Task>();

...

_list.Add(DoStuff(1));
_list.Add(DoStuff(2));
_list.Add(DoStuff(3));

Then whenever you need to see how many are remaining, you can just do this:

list.Count(a => !a.IsCompleted);

Essentially, IsCompleted is true only if that thread finished to completion. However, you may also need to check for IsFaulted or IsCanceled, depending on what those tasks are doing.

***Edit Update 2

On the thread that is adding to this list, you can then remove them like this with no worries:

_list.Add(DoStuff(4));
_list.Add(DoStuff(5));

_list = _list.Where(a => !a.IsCompleted).ToList();

If you don't want to mess with the actual variable reference, you can do a backwards for loop and remove manually so long as another thread isn't adding to this list.

Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

Why is locking needed?
I need them to run in the background and for the calling method to continue executing without awaiting them. They are being added continuously by while loop so I can't await. I just need to make sure that after any task is completed it will be removed from the list.
When you Remove from the dictionary, there is some sort of underlying collection that if you removed in the middle, your index could go out of range an another. Whenever you work with threads, always lock everything that needs to mutate anything.
@DanielLorenz thank you, yes I thought about it already. I could call that method from time to time and remove all tasks that are already completed. I just thought that the solution when task is removed as soon as it finishes is more beautiful, but if that will include mandatory locking then I'm not sure already. Anyway I suspected that some underlying connection can be messed up by threads, thanks for confirming this!
@EwanCoder If you only have one thread adding new ones, that same thread can remove the Completed ones with on risk. Only if you try to remove them on a child thread will you have problems. See Update 2 if you'd like! :)
1

The Dictionary class is not thread-safe, so you definitely need to protect it with a lock if you intend to manipulate it by multiple threads in parallel. Otherwise the internal state of the class may become corrupted.

Regarding the performance, it shouldn't be an issue. Deleting an item from a Dictionary is very fast, so if you hold the lock only for the duration if this operation, the contention for the lock should be near to non-existent. You should start worrying only if you anticipate a frequency of around ~100,000 dictionary operations per second or more. In that case it would be wise to switch to a ConcurrentDictionary, which will handle better the increasing contention because of its granular locking implementation.

There is race condition in your implementation though. A Task is added into the dictionary after its creation, so it is theoretically possible that it will try to remove itself before it's even there. If this happens, you may end up with completed tasks staying in the dictionary forever. This is not an easy problem to fix in an elegant way.

Improve this answer

1 Comment

Hm thank you very much! I don't anticipate so high frequency. But I didn't think about that race condition! Thanks for pointing it out.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.