The reason for the difference between TXPSubject.DeleteObserver and TXPSubject.DeleteObservers in DUnit's XPObserver.pas?

Question

I find it hard to understand the reason why implementations of TXPSubject.DeleteObserver and TXPSubject.DeleteObservers in XPObserver.pas are different? Specifically, the different order of "Dispose" and "ReleaseSubject" calls, and the reasoning why the different order matters for TXPSubject.DeleteObserver. The code in question is extracted and shown below. The file is available from DUnit.

function TXPSubject.DeleteObserver(const Observer: IXPObserver;
  const Context: pointer): boolean;
var
  idx: integer;
  ObserverInfo: TXPObserverInfo;

begin
  FSync.Enter;

  try
    // Check for existence or prior removal
    Result := FindObserver(Observer, Context, idx);

    if Result then
    begin
      // Need local ref after deletion from list. Order of Delete() &
      // ReleaseSubject() is important here for correct functioning of _Release
      // ...***DON'T*** refactor this method!!
      ObserverInfo := PXPObserverInfo(FObservers[idx])^;
      // Release our (list) reference to observer
      PXPObserverInfo(FObservers[idx])^.Observer := nil;
      System.Dispose(FObservers[idx]);
      FObservers.Delete(idx);
    end;

    // Exit critical section here as we now have local vars only (thread-safe)
    // and call to ReleaseSubject below on last reference will leave FSync
    // invalid (destroyed).
  finally
    FSync.Leave;
  end;

  // Notify Observer to release reference to us. This will result in
  // a call to TXPSubject._Release.
  if Result then
    ObserverInfo.Observer.ReleaseSubject(IXPSubject(ObserverInfo.Subject),
      ObserverInfo.Context);

end;


procedure TXPSubject.DeleteObservers;
var
  idx: integer;
  ObserverInfo: PXPObserverInfo;

begin
  FDeletingObservers := true;
  // Count *down* to allow for side-effect of loop actions -
  // referenced item will be deleted from list, and remainder will move down
  // one slot.
  for idx := FObservers.Count - 1 downto 0 do
  begin
    ObserverInfo := FObservers[idx];
    // Notify Observer to release reference to Subject
    ObserverInfo^.Observer.ReleaseSubject(IXPSubject(ObserverInfo.Subject),
      ObserverInfo^.Context);
    // Release our (list) reference to Observer
    ObserverInfo^.Observer := nil;
    System.Dispose(ObserverInfo);
    FObservers.Delete(idx);
  end;

  FDeletingObservers := false;
end;

Answer 1

The second method is the more obvious implementation, which does not bother with thread-safety. The first one is the tricky one.

That first method uses a critical section FSync for thread-safety. Since TXPSubject is a TInterfacedObject (which cannot be seen in the provided code), it will be destroyed when the last reference to that object is dropped. That means that when the last observer is deleted using DeleteObserver which calls ReleaseSubject on that last observer (which should set the reference to the subject to nil), the object will be destroyed. Now if the ReleaseSubject would have been called within the lock, then the line

FSync.Leave;

causes an access violation because the object (and therefore also FSync) has been destroyed already.

So instead of calling ReleaseSubject from within the lock, a local copy of the observer info is created with

ObserverInfo := PXPObserverInfo(FObservers[idx])^;

This dereferences the pointer stored in the observer list and basically creates a local copy. Then, after the FSync lock has been released, ReleaseSubject is called on that local copy. After that point Self is not referenced any more so it does not matter that the object has been destroyed.

EDIT: Whoever calls DeleteObserver still holds a valid reference to ISubject so it should not get destroyed until this reference falls out of scope. So the scenario described in this answer only happens if the observer itself calls DeleteObserver (probably in its destructor) on the reference that is later reelased by ReleaseSubject.

Answer 2

1) DeleteObserver and DeleteObservers are used in different contexts, client and server, respectively. With the benefit of hindsight, I would now reduce the IXPSubject interface to simply a client interface to the subject

The client interface, used by Observers to register and de-register interest in a Subject:

  IXPSubject = interface(IXPObserver)
    ['{D7E3FD5D-0A70-4095-AF41-433E7E4A9C29}']
    function AddObserver(const Observer: IXPObserver;
      const Subject: IXPSubject; const Context: pointer = nil): boolean;
    function InsertObserver(const idx: integer; const Observer: IXPObserver;
      const Subject: IXPSubject; const Context: pointer = nil): boolean;
    function DeleteObserver(const Observer: IXPObserver;
      const Context: pointer = nil): boolean;

    function ObserverCount: integer;
    function GetObserver(const idx: integer): IXPObserver;
    property Observers[const idx: integer]: IXPObserver read GetObserver;
    property Count: integer read ObserverCount;
  end;

DeleteObservers is only called indirectly, by TXPSubject._Release. This method does not use the critical section, FSync, because it's use within _Release is already guarded by FSync. DeleteObservers should really be a private method on the class implementing IXPSubject, TXPSubject

You'll notice that DeleteObservers is called conditionally in TXPSubject._Release. This is when the primary reference to the Subject, created before any Observers are registered, is being released. Typically, this will be when the container object for the Subject is being destroyed.

2) Ordering of methods

In TXPSubject.DeleteObserver:

  // Release our (list) reference to observer
  PXPObserverInfo(FObservers[idx])^.Observer := nil;
  System.Dispose(FObservers[idx]);
  FObservers.Delete(idx);
  ObserverInfo.Observer.ReleaseSubject(IXPSubject(ObserverInfo.Subject),
    ObserverInfo.Context);

In TXPSubject.DeleteObservers:

  // Notify Observer to release reference to Subject
  ObserverInfo^.Observer.ReleaseSubject(IXPSubject(ObserverInfo.Subject),
    ObserverInfo^.Context);
  // Release our (list) reference to Observer
  ObserverInfo^.Observer := nil;
  System.Dispose(ObserverInfo);

Observer-initiated de-registration

As mentioned earlier, Subject.DeleteObserver is called by an Observer when it wishes to to de-register itself from Subject - when the Observer initiates the de-registration process. The effect of this is for the Subject to finalise its internal state (removing references to Observer), before it calls (temp thread-local copy of) Observer.ReleaseSubject

In Observer.ReleaseSubject, the Observer's reference to Subject is nil'd, indirectly causing a call to Subject._Release...

TXPSubject._Release checks to see if the primary reference to itself is being released. If we had not finalised the internal state of the Subject before calling Observer.ReleaseSubject, we would now, erroneously, call DeleteObservers. The correct behaviour in this situation requires that ReleaseSubject is called last.

Subject-initiated de-registration

The Subject initiates de-registration when the primary reference is being released. As mentioned earlier, typically, this will be when the container object for the Subject is being destroyed, or a singleton (possibly unit-scoped or global) reference is destroyed on application shutdown.

We can see in TXPSubject._Release, that DeleteObservers will be called in this situation. The flag FDeletingObservers is set on entry to DeleteObservers to prevent re-entrancy when Observers releasing their references to Subject indirectly call TXPSubject._Release.

Subject must have a reference to Observer to be able to call Observer.ReleaseSubject, so the finalising of internal state occurs after the call to Observer.ReleaseSubject in DeleteObservers.

I hope this helps to clarify the situation for you Li?

Cheers, Paul.