Double checked locking

[Attila Kovacs 617]

I came across this ineresting post https://stackoverflow.com/questions/27368556/trtticontext-multi-thread-issue after getting an error report over an AV in System.Rtti.MakeClosure

and I'm wondering, according to the answers from @Stefan Glienke and @David Heffernan if the "data := nil;" in System.Rtti.LazyLoadAttributes,MakeClosure should not be the first after

the second check?

 

[Stefan Glienke 1905]

Why exactly? data is the PByte parameter that comes into that function and if ultimately not nil it gets passed to ConstructAttributes

[Dalija Prasnikar 1341]

15 hours ago, Attila Kovacs said:

I came across this ineresting post https://stackoverflow.com/questions/27368556/trtticontext-multi-thread-issue after getting an error report over an AV in System.Rtti.MakeClosure

and I'm wondering, according to the answers from @Stefan Glienke and @David Heffernan if the "data := nil;" in System.Rtti.LazyLoadAttributes,MakeClosure should not be the first after

the second check?

 

First of all, when asking about particular issue specifying Delphi version is mandatory. System.Rtti is being updated for literally every release (including updates). If you want us to comment on source code we need to make sure that we are looking at the same code.

 

The code in question is perfectly fine. But the whole thing is broken in another place. Namely procedure TRttiInstanceType.ReadPropData; and other similar methods which use boolean for checking, but without double checked locking. This can causes calling LazyLoadAttributes twice which then can corupt the whole thing in that code. For instance I would get a crash on nil Finalizer instance.

 

if FReadPropData then
    Exit;

  p := PByte(TypeData.PropData);

  TMonitor.Enter(Package.FLock);
  try
   // Here we would need to check FReadPropData again end exit if it is true 
    classic := ReadClassicProps;
    ext := ReadExtendedProps;

    FProps := SubtractClassic(classic, ext);

    FAttributeGetter := LazyLoadAttributes(p);
    FIndexedProps := ReadIndexedProps;
    FReadPropData := True;
  finally
    TMonitor.Exit(Package.FLock);
  end;
end;

 

Edited September 18, 2023 by Dalija Prasnikar

[Dalija Prasnikar 1341]

I reported the issue as https://quality.embarcadero.com/browse/RSP-42359

[Attila Kovacs 617]

55 minutes ago, Dalija Prasnikar said:

First of all, when asking about particular issue specifying Delphi version is mandatory. System.Rtti is being updated for literally every release (including updates). If you want us to comment on source code we need to make sure that we are looking at the same code.

I checked it and it's still the same, I should have mention it tough.

55 minutes ago, Dalija Prasnikar said:

The code in question is perfectly fine. But the whole thing is broken in another place.

Ouch. Now that you say, nice one.

 

So basically, my approach to create/try-finally/free the TRtttiContext, which is advocated by some, is also wrong because of the faulty library, and I should instantiate one somewhere at the beginning?

 

 

Edited September 18, 2023 by Attila Kovacs

[Dalija Prasnikar 1341]

7 minutes ago, Attila Kovacs said:

So basically, my approach to create/try-finally/free the TRtttiContext, which is advocated by some, is wrong because of the faulty library, and I should instantiate one somewhere at the beginning?

Instantiating TRttiContext at the beginning would not help in this case as the problem is in additional lazy loading. Also not creating global TRttiContext is also affected. Whatever you do it will be broken.

 

However, depending on the Delphi version you are using, creation of global TRttiContext solves some other threading issues. And it is also faster than doing create... try..finally.. free approach. 

 

You can patch System.Rtti to fix the issue.

[Dalija Prasnikar 1341]

35 minutes ago, Attila Kovacs said:

I checked it and it's still the same, I should have mention it tough.

Even if this particular piece of code is the same, some other part critical for reproducing the issue might be different. So knowing the Delphi version is always important.