Resolve a random race condition

[jenniferDomer]

We are randomly seeing this error. We think it is a race condition that can be resolved with locking, but for now this is a small change to make.

[joaomatossilva]

I see that error sometimes when 2 holidays fall into the same date.

I can see from stacktrace that you are using the en-US culture. what year was it? 2018?

[joaomatossilva]

Can you change this class to use a ConcurrentDictionary instead?

This error can happen since the GetInstance Method is not thread safe because of the internal dictionary.

[darrencauthon]

I looked at ConcurrentDictionary and it seems that the .Add is replaced with .AddOrUpdate

And....

The code could be flipped to use ConcurrentDictionary but it will have the same effect as replacing .Add(key, value) with [key] = value. The combination of the functions passed to AddOrReplace will result in replacing the previous value with the new value.

As a general rule of thumb, I suggest that developers be very careful with .Add on a dictionary. To the reader it looks like the developer is just assigning a value to a key, shorthand for this:

dictionary[key] = value; // synonymous with dictionary.Add(key, value);

but in reality it is:

if (dictionary.ContainsKey(key)) throw new Exception("An item with this key has already been added");
dictionary[key] = value;

It's very rare when "an item with this key has already been added" was desired. In my experience, the developer's expectation is that the value should overwrite the existing value.

[joaomatossilva]

Actually, I was more interested on the GetOrAdd method.

https://andrewlock.net/making-getoradd-on-concurrentdictionary-thread-safe-using-lazy/

[joaomatossilva]

@jenniferDomer @darrencauthon So, have you looked into the link I've sent you?

I would really love in something that got guarantee the factory method would only be called once, since the date calculation can be "heavy" on some holidays.
The ConcurrentDictionary doens't guarantee that, that's why I've shared that link, so you could contribute here with a proper solution.

Do you have any way to efectively replicate the error?

[darrencauthon]

@joaomatossilva My personal opinion is that it would be better to try to keep things as simple as possible. We've found this issue with thread safety... but it's thread safety on a race to insert the same value into the same point. No matter if you take this PR or you go with some sort of thread-safe change, the end result will be exactly the same.

If you want to address thread-safety, then I would recommend some refactors to the project first. I'd say it could be refactored to operate in a more non-static form, then expose the features you want as static methods that wire up to a singleton. Then you could resolve the thread-safety issues in a single seam instead of patching it here-and-there into the project.

[joaomatossilva]

it's thread safety on a race to insert the same value into the same point

The point is more the "same value". That's the thing I do want to avoid, because that's the most potentially resource expensive operation.

That's why I want the value calculation ensure to run only once. The race condition is only a symptom.

[darrencauthon]

It's ok to treat symptoms of a larger problem, especially if they could cause issues of their own.

And especially if they're easy to resolve. Using [] instead of .Add will result in the users of this library to not have to worry about random runtime errors.

There is a race condition here, sure. The fix isn't ConcurrentDictionary, though... that's a tree. You need a forest-level look at how you are doing work within a static context, and I think you'll find a solution that solves not just this particular issues, but all of them. I haven't checked this entire library but I bet this is appearing in other operations, too.