Thread contention in TEntityCache, Causes Access Violations Errors

When multiple threads are trying to get an entity cache entry for the same Entity type, and they arrive at the TEntityCache.Get method around the same time, Access violations can occur.

Please note the following Code. If more than one thread is contending for the same entity cache entry and this cache entry has not been previously created, the first thread that enters the critical section will update the dictionary with it's instance and then release the critical section. It will then return it's instance of the interfaced cache entry to the calling process. Then when the next thread enters the critical section, because it has already passed the TryGet with a false it will then replace the first threads instance of the class and update the dictionary entry with it's instance of the cache entry.

At some point the first threads Result gets referenced counted down to zero and destroyed, but the first thread is not done using it yet, causing Access Violations.

class function TEntityCache.Get(entityClass: TClass): TEntityData;
begin
  if not TryGet(entityClass, Result) then
  begin
    Result := TEntityData.Create;
    Result.SetEntityData(entityClass);
    fCriticalSection.Enter;
    try
       fEntities.AddOrSetValue(entityClass, Result);
    finally
      fCriticalSection.Leave;
    end;
  end;
end;

In order to resolve this issue we modified the code as follows moving up the Critical Section to the top of the method.

class function TEntityCache.Get(entityClass: TClass): TEntityData;
begin
  fCriticalSection.Enter;
  try
    if not TryGet(entityClass, Result) then
    begin
        Result := TEntityData.Create;
        Result.SetEntityData(entityClass);
        fEntities.AddOrSetValue(entityClass, Result);
    end;
  finally
    fCriticalSection.Leave;
  end;
end;

Now when the first thread enters the method and the TryGet returns false, it is the only thread that creates the entry as all other threads are blocked at the top of the method. Once the first thread releases the critical Section then the next thread enters and the tryGet returns True so no additional instances of the same cache entry are created.

Comments (3)