TMappingExplorerManager threading issue

BIZ TMS Aurelius
1

It looks like TMappingExplorerManager isn't working in a thread-safe way:


function TMappingExplorerManager.Get(ModelName: string): TMappingExplorer;
begin
  ModelName := LowerCase(ModelName);
  if FExplorers.TryGetValue(ModelName, Result) then Exit;
  Result := TMappingExplorer.Create(ModelName);
  FExplorers.Add(ModelName, Result);
end;

If the application spawns several threads that each created new TObjectManagers without having first having created at least one in the main thread (thus, initializing model), there is a very good chance that the above code will fail: TryGetValue will be false for the first few threads without any of them having had time to reach the FExplorers.Add(ModelName, Result)

That whole section should be protected by a critical section.

The same goes for the way the class variable FExplorerManager: TMappingExplorerManager; of TMappingExplorer is accessed: 

class function TMappingExplorer.ExplorerManager: TMappingExplorerManager;
begin
  if FExplorerManager = nil then
    FExplorerManager := TMappingExplorerManager.Create;
  Result := FExplorerManager;
end;

This has a high risk of causing several different TMappingExplorerManager to be created with the reference to all of them but the last one being lost (but still used by calling threads).

This is a problem because TObjectManager isn't thread-safe (which is fine) and therefore needs to be re-created for each concurrent threads. Each of these instances will attempt to access the TMappingExplorerManager singleton in order to obtain its one instance of TMappingExplorer and trigger the race condition.
2

That's true, but adding a critical section will slow down the process. It's better that you just initialize all explorers you will need in your application before spawning the threads. You can also use the following code to initialize the explorer and make it fully thread-safe:



E := TMapingExplorer.Get(MyModelName);

for C in E.Hierarchy.Classes do
begin
  E.GetTable(C);
  E.GetId(C);
  E.GetVersionColumn(C);
  E.GetAssociations(C, False, False);
  E.GetAssociations(C, False, True);
  E.GetAssociations(C, True, False);
  E.GetAssociations(C, True, True);
  E.GetColumns(C, False, False);
  E.GetColumns(C, False, True);
  E.GetColumns(C, True, False);
  E.GetColumns(C, True, True);
  E.GetDiscriminatorColumn(C, False);
  E.GetDiscriminatorColumn(C, True);
  E.GetSequence(C, False);
  E.GetSequence(C, True);
  E.GetClassStateMembers(C, False, False);
  E.GetClassStateMembers(C, False, True);
  E.GetClassStateMembers(C, True, False);
  E.GetClassStateMembers(C, True, True);
  E.GetClassVisibleMembers(C, False);
  E.GetClassVisibleMembers(C, True);
end;
Wagner R. Landgraf2018-06-10 02:33:07
3

Thanks. I will do something like this.


Using a CS doesn't have to be slow, though: check for nil outside the CS and, it's it's nil, then lock the CS and check it again: that way, you'll be paying the performance price only for the time when it's needed (during concurrent initializations).