TRTTIContext multi-thread issue

I think I found the problem. It is inside TRealPackage.FindType and MakeTypeLookupTable.

MakeTypeLookupTable checks for FNameToType being assigned. If not it runs DoMake. This one is protected with TMonitor and checks FNameToType being assigned again after entering.

So far so good. But then happens the mistake as inside DoMake FNameToType gets assigned causing other threads to happily pass MakeTypeLookupTable and return to FindType which then does return false in FNameToType.TryGetValue and returns nil.

Fix :

Since FNameToType is used outside of the locked DoMake as indicator that execution can continue it should not be assigned inside DoMake until it's properly filled up.

Edit: Reported as https://quality.embarcadero.com/browse/RSP-9815

Most recently (as of 2019-Nov) marked as Fixed in Delphi 10.3 Rio.

As Stefan explains, the problem is down to a faulty implementation of the double checked locking pattern. I'd like to expand on his answer and try to make it more clear what is wrong.

The erroneous code looks like this:

procedure TRealPackage.MakeTypeLookupTable;

  procedure DoMake;
  begin
    TMonitor.Enter(Flock);
    try
      if FNameToType <> nil then // presumes double-checked locking ok
        Exit;

      FNameToType := TDictionary<string,PTypeInfo>.Create;
      // .... code removed from snippet that populates FNameToType
    finally
      TMonitor.Exit(Flock);
    end;
  end;

begin
  if FNameToType <> nil then
    Exit;
  DoMake;
end;

The fault is that the code that populates the shared resource FNameToType is executed after FNameToType has been assigned. That code which populates the shared resource needs to execute before FNameToType is assigned.

Consider two threads, A and B. They are the first threads to call MakeTypeLookupTable. Thread A arrives first, finds that FNameToType is nil and calls DoMake. Thread A acquires the lock and reaches the code that assigns FNameToType. Now, before thread A manages to run any more code, thread B arrives in MakeTypeLookupTable. It tests FNameToType and finds that it is not nil, and so returns immediately. The calling code then makes use of FNameToType. However, FNameToType is not yet in a fit state to use. It has not been populated because thread A has not yet returned.

The most obvious fix from Embarcadero's side looks like this:

procedure DoMake;
var
  LNameToType: TDictionary<string,PTypeInfo>;
begin
  TMonitor.Enter(Flock);
  try
    if FNameToType <> nil then // presumes double-checked locking ok
      Exit;

    LNameToType := TDictionary<string,PTypeInfo>.Create;
    // .... populate LNameToType
    FNameToType := LNameToType;
  finally
    TMonitor.Exit(Flock);
  end;
end;

However, take note of the comment that says presumes double-checked locking ok. Well, double checked locking is fine when the machine has a strong enough memory model. So it's all good on x86 and x64. But ARM has a relatively weak memory model. So I have strong doubts as to whether or not this fix is enough on ARM. Indeed I do wonder where else in the RTL that Embarcadero have used double checked locking.

If TRealPackage had been declared in the interface section of the code then it would be easy enough to patch TRealPackage.MakeTypeLookupTable to apply the change above. However, that is not the case. So in order to apply a work around I suggest the following:

1. Use a single global RTTI context for all your RTTI code.
2. In the initialization stage of your program, make a call on that context that in turn forces a call to TRealPackage.MakeTypeLookupTable. Because initialization happens single threaded you avoid the race condition.

Declare the global context like this, say:

var
  ctx: TRttiContext;

And force the call to TRealPackage.MakeTypeLookupTable like this:

ctx.FindType('');

So long as all your RTTI code goes via this single shared context then you cannot fall foul of this race.