Locking an Object

[fastbike 0]

We have a global object to manage a list of long lived objects that are created by reading an XML definition file from disk and parsing it to create an in memory structure.

 

The global object itself uses a lazy creation pattern as it is possible that the functionality contained within may not be accessed during the lifetime of the application (web server hosted under IIS with a 2 hour recycle time).

 

The lazy creation pattern checks to see if the object exists, if not it is created, then it initializes the items from the xml definition files.  While it is being initialised we want to prevent any other thread from accessing the methods.

Is the TMonitor  record going to be useful here - i.e. if the first thread called TMonitor.Enter(Self) on the object would that prevent another thread from making the same call to the object before TMonitor.Exit(Self) had been called by the first thread.

 

The compiler is Delphi 11.3 Enterprise.  I can post some code but I'm trying to understand in more general terms  how we can lock the object so only one thread can access it first.

[pyscripter 694]

2 hours ago, fastbike said:

if the first thread called TMonitor.Enter(Self) on the object would that prevent another thread from making the same call to the object before TMonitor.Exit(Self) had been called by the first thread.

Indeed.  This is the whole purpose of locking.

[fastbike 0]

1 hour ago, pyscripter said:

Indeed.  This is the whole purpose of locking.

I think I worded that part of my question poorly in retrospect.

 

If one thread is calling a method to initialize/load the objects, what is required to prevent a second thread from accessing the global object via another method that tries to read from the list of items.

I'm thinking I need a Tmonitor.wait loop in that second method that waits for the class level flag to be set by the initialize method after it has finished loading the items.

I will post some code snippets tomorrow.

[Kas Ob. 124]

11 minutes ago, fastbike said:

If one thread is calling a method to initialize/load the objects, what is required to prevent a second thread from accessing the global object via another method that tries to read from the list of items.

I'm thinking I need a Tmonitor.wait loop in that second method that waits for the class level flag to be set by the initialize method after it has finished loading the items.

Remove all an be done in few ways, the easiest and simplest one is to centralize the access to that object and don't access it directly, use a getter, that getter can be function in singleton or global one, or just a property with getter, this getter will have CriticalSection instead of TMonitor, i just hate TMonitor, it is up to you after all, both will do fine.

In that getter the locked section will active only if the field or the object is assigned meaning it will not cause locking once it is created, but if it is not yet assigned, the lock will be holding all threads except one, the creator, once it created and assigned the new callers will not enter locked section, and the waiting ones will be released, but the first check in the locked section will be to see if the object is assigned and created then exit.

 

Just make sure the creating of that object doesn't fail or wait indefinitely.

Something like this , more or less

function TForm10.GetMyObject: TMyObject;
var
  TempMyObject: TMyObject;
begin
  Result := FMyObject;
  if Assigned(Result) then
    Exit;

  FCriticalSection.Enter;
  try
    Result := FMyObject;
    if Assigned(Result) then
      Exit;

    //  Create MyObject in TempMyObject , DON'T CREATE FMyObject directly

    FMyObject := TempMyObject;
    Result := FMyObject;
  finally
    FCriticalSection.Release;
  end;
end;

 

[Dalija Prasnikar 1404]

32 minutes ago, Kas Ob. said:

that getter can be function in singleton or global one, or just a property with getter, this getter will have CriticalSection instead of TMonitor, i just hate TMonitor, it is up to you after all, both will do fine.

You cannot use TMonitor in such scenario as you don't always have live instance to call upon. 

 

Also there is a simpler code than your example commonly used for double checked locking. However, that pattern assumes that calling the constructor is all that is needed to fully initialize the object. Only if there is additional initialization needed after construction, there is a need for temporary object.

 

So this code would look like:

 

function GetMyObject: TMyObject;
begin
  if FMyObject = nil then
    begin
      FLock.Enter;
      try
        if FMyObject = nil then
          FMyObject := TMyObject.Create;
      finally
        FLock.Leave;
      end;
    end;
  Result := FMyObject;
end;

or with additional initialization

 

function GetMyObject: TMyObject;
var
  TempMyObject: TMyObject;
begin
  if FMyObject = nil then
    begin
      FLock.Enter;
      try
        if FMyObject = nil then
          begin
            TempMyObject := TMyObject.Create;
            // additional initialization for TempMyObject

            FMyObject := TempMyObject;
          end;
      finally
        FLock.Leave;
      end;
    end;
  Result := FMyObject;
end;

 

 

[Kas Ob. 124]

50 minutes ago, Dalija Prasnikar said:

Also there is a simpler code than your example commonly used for double checked locking.

My aim was the most readable code for OP to make sure he get the idea and enhance on it as he see fit.

[fastbike 0]

Thanks for the comments and suggestions. I'll copy in some simplified code snippets - I think I have misunderstood how TMonitor would protect multiple threads from accessing methods of  a global object.

 

I'm trying to post an example here and keep getting a message saying my post appears to be spam !

I'll try posting each separately so apologies in advance for having to read through it all.

[fastbike 0]

Here's the declaration of the type and a global function to an instance of that type

// interface to provide functionality that will be implemented by a global object
type
  /// <summary>Register of supported FHIR Operations</summary>
  IFHIROperationFactory = interface
    ['{6E207FA9-CBB8-4F7A-A54F-D90621B601C8}']
    function GetOperation(Id: string): IFHIROperationDefintion;
    procedure Reload;
  end;

// a global pointer to a function that returns an instance (allows mocking for testing other parts of the code)
var
  FHIROperationFactory: function: IFHIROperationFactory;

 

[fastbike 0]

Next up there is a default implementation which can read xml files from  disk and create a list of IFHIROperationDefintion (worker) objects.

// concrete class declared in the implementation part of the unit, so only accessible via the global function
type
/// <summary>Concrete class for FHIR Operations </summary>
  TFHIROperationFactory = class(TInterfacedObject, IFHIROperationFactory)
  private
    FOperationsLoaded: Boolean;
    FRegisteredOperations: TDictionary<string, IFHIROperationDefintion>;
    procedure LoadOperationDefinitions;
  protected
    { IFHIROperationFactory }
    function GetOperation(Id: string): IFHIROperationDefintion;
    procedure Reload;
  public
    constructor Create;
    destructor Destroy; override;
  end;

 

[pyscripter 694]

Here is a lock-free implementation for your FHIROperationFactory function.

var
  SingletonFHIROperationFactory : IFHIROperationFactory

function FHIROperationFactory: IFHIROperationFactory;
var
  LFHIROperationFactory: IFHIROperationFactory;
begin
  if SingletonFHIROperationFactory = nil then
  begin
    LFHIROperationFactory :=  TFHIROperationFactory.Create;
    if InterlockedCompareExchangePointer(Pointer(SingletonFHIROperationFactory),
      Pointer(LFHIROperationFactory), nil) = nil
    then
      SingletonFHIROperationFactory._AddRef;
  end;
  Result := SingletonFHIROperationFactory;
end;

Delphi does something similar in some places.

[fastbike 0]

6 hours ago, pyscripter said:

Delphi does something similar in some places.

I have a similar implementation but am having trouble posting replies to this thread ion this forum. It seems the filters are rejecting my posts for reasons unknown.

[fastbike 0]

My implementation, very similar but with a function that @Remy Lebeau helped me write.

 

The singleton can only be accessed via the global function pointer as all of this code is in the implementation section.

 

// global singleton variables/methods
var
  _FHIROperationFactory: IFHIROperationFactory;

function GetDefaultFHIROperationFactory: IFHIROperationFactory;
var
  newFHIRFactory: IFHIROperationFactory;
begin
  if _FHIROperationFactory = nil then
  begin
    { The object does not exist yet. Create one. }
    newFHIRFactory := TFHIROperationFactory.Create;
    { It is possible another thread also created one, so get the first}
    InterlockedCompareExchangeIntf(IInterface(_FHIROperationFactory), newFHIRFactory, nil);
    _FHIROperationFactory.Reload;
  end;
  Result := _FHIROperationFactory;
end;

initialization
// assign an implemntation method to the global function pointer
FHIROperationFactory := GetDefaultFHIROperationFactory;

end.

 

[fastbike 0]

Some vanilla code for constructor / destructor

constructor TFHIROperationFactory.Create;
begin
  inherited Create;
  FRegisteredOperations := TDictionary<string, IFHIROperationDefintion>.Create;
end;

destructor TFHIROperationFactory.Destroy;
begin
  FRegisteredOperations.Free;
  inherited;
end;

[Click and drag to move]

 

[fastbike 0]

And the code that does the initial loading / reloading of the xml files and creation of the implementation objects

///<summary>Load (or reload) the OperationDefinition resources from *.xml files</summary>
procedure TFHIROperationFactory.Reload;
begin
  FOperationsLoaded := false;
  TMonitor.Enter(Self);
  try
    LoadOperationDefinitions; // a helper method that reads files from a directory, creates objects and adds to the dictionary
    FOperationsLoaded := true;
  finally
    TMonitor.Exit(Self);
  end;
end;

 

[fastbike 0]

And the code that gets called by other worker threads when they need a IFHIROperationDefintion instance. I think the TMonitor code is of no effect - in hindsight it should actually be sitting there and spinning (via  TMonitor.Wait ? ) until the Reload method has set the lock variable (FOperationsLoaded). I'll provide an improved implementation of these two methods in the next post for inspection and comment.

 

function TFHIROperationFactory.GetOperation(Id: string): IFHIROperationDefintion;
begin
  if not FOperationsLoaded then
  begin
    Log.Debug('Waiting for Operations to be loaded', DefaultLogTag);
    TMonitor.Enter(Self);
    try
      if not FOperationsLoaded then
      begin
        Reload;
      end;
    finally
      TMonitor.Exit(Self);
    end;
  end;

  if not FRegisteredOperations.TryGetValue(Id, Result) then
  begin
    raise EFHIRExceptionServer.CreateFmt('FHIR Operation with id %s not registered', [Id]);
  end;
end;

 

Edited November 14, 2023 by fastbike

[fastbike 0]

An improved version of the Reload method (called either independently via an URL endpoint) or as part of the lazy creation of the object (see above)

This one does a pulse all to advise any waiting threads that the global objects is now unlocked.

/// <summary>Load (or reload) the OperationDefinition resources from *.xml files, and populate the type mapper</summary>
procedure TFHIROperationFactory.Reload;
begin
  TMonitor.Enter(Self);
  try
    FOperationsLoaded := false;
    LoadOperationDefinitions;
    FOperationsLoaded := true;
    TMonitor.PulseAll(Self);
  finally
    TMonitor.Exit(Self);
  end;
end;

 

[fastbike 0]

And finally a much improved version of the method that should not return until the Reload method above has done its thing.

function TFHIROperationFactory.GetOperation(Id: string): IFHIROperationDefintion;
begin
  Log.Debug('GetOperation for %s', [Id], DefaultLogTag);

  if not FOperationsLoaded then
  begin
    Log.Debug('Waiting for Operations to be loaded', DefaultLogTag);
    TMonitor.Enter(Self);
    try
      for var I := 0 to Settings.ReadInteger('Global', 'RetryOperationsReload', 15) do // prevent run away loop
      begin
        while not FOperationsLoaded do
          TMonitor.Wait(Self, 250);
      end;
      if not FOperationsLoaded then
        raise EFHIRExceptionServer.Create('Could not load FHIR Operations, check settings Global|RetryOperationsReload');
    finally
      TMonitor.Exit(Self);
    end;
  end;

  if not FRegisteredOperations.TryGetValue(Id, Result) then
  begin
    raise EFHIRExceptionServer.CreateFmt('FHIR Operation with id %s not registered', [Id]);
  end;
end;

 

[fastbike 0]

And now that the forum filters have relaxed, am I using the TMonitor record correctly to lock the global object until it has been populated ?

[Kas Ob. 124]

Well, many many things and questions went into my head after seeing your snippets code.

 

Why are you complicating things ?

1) As one you can just create a new and initialize it then replace it without locking at all, that is possible because your thread are running without explicit control like just use this one, so create new one and replace it, simple like that.

2) From your first post, i got the idea that the create and reload will be one time and one time only, but it seems you will create it once and reload many times.

3) in "TFHIROperationFactory.GetOperation" there is no comment showing were the result is filled .

4) in that same function your log saying "Waiting for Operations to be loaded" while it should say "try to load or wait"

5) You are accessing some IO operation i think, deducing that from the name "Settings.ReadInteger", most likely some IniFile, File, DB operation... irrelevant per se, but why doing it from locked section, halting all your threads, such operation can be done by one background thread on interval that satisfy your application need, again this valid operation and valid point due the way your threads are running, because from what i see you are letting the threads handling what every there, in different situation, all the threads are waiting on signal to start processing, but here you are just changing lanes for them, so reading the settings and apply with touching or blocking threads is valid point.

6) I don't like the following at all

3 hours ago, fastbike said:

/// <summary>Load (or reload) the OperationDefinition resources from *.xml files, and populate the type mapper</summary>
procedure TFHIROperationFactory.Reload;
begin
  TMonitor.Enter(Self);
  try
    FOperationsLoaded := false;
    LoadOperationDefinitions;
    FOperationsLoaded := true;
    TMonitor.PulseAll(Self);
  finally
    TMonitor.Exit(Self);
  end;
end;

 

What the point of it if "TFHIROperationFactory.GetOperation" is using the same TMonitor.Enter(Self) ?!!

 

These are the obvious points but i see few others, anyway,,,

Don't complicate things for your self when it is absolutely not needed, redesign that in very simple way, your threads are consuming whatever they are fed, great and perfect for lock-free design, create and initialize another (temp) then perform all the operation you need like load and reload, then just swap it, to be on safe side you can use atomic operation to signal for you self that this line is thread critical line, now for the old, free it when its ref count is 0, if you are using refCounted Interfaces then this should happen automatically for you, if not then just add RefCount field for it, lets consumer threads add and dec it, then make sure the code will check if this is new and there is an old, (here the temp mentioned above will be not temp but temp field), if it has assigned value there this means the thread that will reach 0 by decreasing and the value is not null will signal a timer or a back ground thread to free it after some time.

 

Hope that helps.

[David Heffernan 2353]

If the methods here can all be called arbitrarily from multiple threads then you seem to have at the very least a race on FOperationsLoaded. I don't understand what that call to Wait is for either. 

[fastbike 0]

16 hours ago, David Heffernan said:

If the methods here can all be called arbitrarily from multiple threads then you seem to have at the very least a race on FOperationsLoaded. I don't understand what that call to Wait is for either. 

There is one global object.

It has a method that loads the data into it (Reload). There is an internal flag to say if the data has been loaded.

There is another method to access that data (GetOperation) - client threads (from IIS so I do not create these consumer threads) call this method which needs to block until the data has been loaded which is signified by the internal flag being set.

Is TMonitor the correct synchronisation structure here ?

 

[David Heffernan 2353]

7 hours ago, fastbike said:

Is TMonitor the correct synchronisation structure here ?

It's a mutex. One thread at a time through protected blocks. Is it the right structure? Maybe. But using a mutex is only going to be useful if you do it right. Do you see the code that accesses FOperationsLoaded outside the mutex. What's your rationale there? Anyway it's kinda hard to analyse your code with it spread over loads of different posts. 

[fastbike 0]

11 hours ago, David Heffernan said:

Anyway it's kinda hard to analyse your code with it spread over loads of different posts. 

Blame the scattered posts on the forum software consistently refusing to allow me to post anything here two days ago - each time posts with codeblocks were tagged as spam and after editing the attempted post I received notification that I had exceeded the number of posts - which is just way beyond frustrating.
I will start another thread on Stackoverflow since this one has become very hard to follow.