Serious concerns about thread safety

[ivankoster]

Hello,

I was interested using this cache and started reading the code to understand it.
I pretty quickly identified a scenario that is probably not threadsafe.
In the class Index<TKey, T>, method GetItem(..)

if ((node?.Value == null) && (creator != null))
{
    ...
}

return node?.Value;

When the method RemoveFromCache() from class Node finishes between the if and return statement in the above snippet, null will be returned from this GetItem() method, because there are no read locks here. I don't think that is the intended behavior?

Also, the remarks for the class FluidCache says there are 2 locks, but I can easily spot 4, in:

• FluidCache
• Index
• LifespanManager
• Node

[dennisdoomen]

Thanks for reporting it. I'll need to dive into it to see if you're right.

[dennisdoomen]

You mean that the node created in GetItem gets removed by RemoveFromCache before GetItem returns?

[ivankoster]

The expression node?.Value occurs twice in the snippet I copy pasted.
If the first expression node?.Value returns non-null, because its in the cache, you advance to below the if statement.
Then another thread that is in RemoveFromCache can invalidate the node.
Then the original thread in GetItem() does the 2nd evaluation of node?.Value.
This time it returns null.

[dennisdoomen]

You're right. The documentation is inconsistent because the concrete GetItem method documents it can return null, but the interface doesn't say that.

[dennisdoomen]

@ivankoster in your opinion, would you expect GetItem to ever return a null under high load?

[ivankoster]

I would expect it to never return null. That would make it consistent with the Base Class Library, for example ConcurrentDictionary.GetOrAdd

[dennisdoomen]

Yeah, I agree. I need to carefully analyze that situation so to not cause any deadlocks (again).