2

I have dictionary:

private static Dictionary<string, List<Event>> eventsForArea;

initialize it in static constructor.

And I have method (I only insert part of the method where I have references to dictionary, but I think it is enough to understand my question.):

    public void Analyze()
    {

        if (!eventsForArea.ContainsKey(Area.Name))
        {                
            lock (eventsForArea)
                eventsForArea.Add(Area.Name, allUnfinished);

            var unknownUnfinished = ..;

            foreach (var @event in unknownUnfinished)
            {                  
                eventsForArea[Area.Name].Remove(@event);
            }
        }
        else
        {
            unfinished = eventsForArea[Area.Name].ToArray();
        }


        foreach (var @event in Events)
        {
            try
            {
                var unfinishedEventsForAsixId = ..;
                if (!@event.CheckIfPresent(registers))
                {
                    foreach (var unfinishedEvent in unfinishedEventsForAsixId)
                    {                            
                        eventsForArea[Area.Name].Remove(unfinishedEvent);
                    }
                }
                else
                {                       
                    eventsForArea[Area.Name].Add(eventWithId);
                }
            }
            catch (Exception e)
            {                
            }
        }
    }

The startup method is asynchronous, it may be run multiple times at the same time. Without lock (eventsForArea) it throw exception:

"Object reference not set to an instance of an object"

Now work, but I dont sure is it enough that there is a lock only in this place?

Silny ToJa
  • 1,815
  • 1
  • 6
  • 20
  • 2
    I'd suggest not using `eventsForArea` as the lock object. See [here](https://stackoverflow.com/q/11775205/3181933). It's probably better to have a single object to lock against that you know you'll never change, so that you know that the lock code will be always valid. – ProgrammingLlama Jan 13 '21 at 09:20
  • 4
    You could use [`ConcurrentDictionary`](https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2?view=net-5.0) – Liam Jan 13 '21 at 09:21
  • 2
    `if (!eventsForArea.ContainsKey(Area.Name)) { lock (eventsForArea) eventsForArea.Add(Area.Name, allUnfinished);` `ContainsKey` is not thread-safe and needs to be inside the `lock`. But yes, the right solution is to use `ConcurrentDictionary` (and likely `ConcurrentBag` rather than `List`). – mjwills Jan 13 '21 at 09:22
  • 1
    `eventsForArea[Area.Name].Remove(@event)` This is equally worrying, since `List.Remove` isn't thread-safe either. – mjwills Jan 13 '21 at 09:23
  • `it throw exception: "Object reference not set to an instance of an object"` I'd expect problems, although not that _specific_ exception. We'd need a [mcve] to comment further. – mjwills Jan 13 '21 at 09:24
  • @mjwills I've seen that exception before when using a list/dictionary in an unsafe way: possibly due to it resizing internally (and the old backing storage becoming null) – canton7 Jan 13 '21 at 09:25
  • @canton7 Interesting. I've seen `IndexOutOfRangeException` more commonly, but I'll assume you are correct. – mjwills Jan 13 '21 at 09:26
  • @mjwills unfortunately, the error did not show anything more, it throws it away when adding the second object to the dictionary. But ConcurrentDictionary it looks like something to solve my problem – Silny ToJa Jan 13 '21 at 09:26
  • 3
    Given the number of issues in your original code (i.e. many), feel free to share your final code in a dotnetfiddle so we can confirm it looks safe. – mjwills Jan 13 '21 at 09:28
  • (or put it up for codereview on the matching stackexchange site, perhaps?) – canton7 Jan 13 '21 at 09:30
  • @mjwills Ok, thanks for the suggestions, but Can you tell me yet. How can I replace the List to keep it safe? – Silny ToJa Jan 13 '21 at 09:31
  • Do you need an ordered list? You can't do better than a `List` with a lock. If you don't need order, use e.g. `ConcurrentBag` – canton7 Jan 13 '21 at 09:34
  • @mjwills sorry, my english isnt good. I thought here you meant that I should also replace the Letter with something else "To be clear - you need to stop using List as well - it isn't thread-safe either." – Silny ToJa Jan 13 '21 at 09:36
  • @mjwills sorry I missed it. Thanks! – Silny ToJa Jan 13 '21 at 09:38
  • 1
    I would recommend reading up on multithreading and threadsafety. It is very easy to get it wrong, and the compiler and processor can reorder code as it wants to as long as it produces the same behavior on a single thread, as long as proper synchronization is not in place. And many if the bugs you can cause can be really difficult to find since they happen rarely, and often only in production. – JonasH Jan 13 '21 at 09:40
  • @mjwills Can I have one more question? I have a dictionary in dictionary, it is enough the first dictionary is ConcurrentDictionary? 'ConcurrentDictionary>' – Silny ToJa Jan 13 '21 at 09:52
  • 1
    @SilnyToJa Check the duplicate. Short answer - if you never _write_ to the `Dictionary` after inserting it in the `ConcurrentDictionary` then you are _probably_ fine. – mjwills Jan 13 '21 at 09:54

0 Answers0