TObservable observer enumeration issue

Stefan Glienke repo owner

Please post some complete sample code to reproduce the issue. Also locks are there to protect access from multiple threads and not to protect from access of the same thread - that would mean a dead lock you know. Notify needs to be changed to iterate over fObservers.ToArray.

2016-06-03T23:20:42+00:00

Ryan Potts reporter

Stefan, I have managed to recreate the issue. The long and the short of it is that an observer is instantiating another form. That action triggers the instantiation and attachment of that forms updater class. This is when the observer list is added to while the list is being enumerated.

Before I came to this understanding I made the following changes to my update monitor class to force attachments/detachments to be buffered. This works fine, but feels kludgy.

    TUpdaterMonitor = class(TObservable<IUpdater>)
    private
      FDapInput : TArray<smallint>;
      FID     : integer;
      FDynamicAttach,
      FDynamicDetach : IList<IUpdater>;
    public
      constructor create;
      destructor Destroy; override;
      procedure DoNotify(const observer: IUpdater); override;
      procedure Attach(const observer: IUpdater);
      procedure Detach(const observer: IUpdater);
      procedure Notify;
      property DapInput: TArray<smallint> write FDapInput;
      property ID: integer write FID;
    end;

constructor TUpdaterMonitor.create;
begin
  inherited create;
  FDynamicAttach := TCollections.CreateList<IUpdater>;
  FDynamicDetach := TCollections.CreateList<IUpdater>;
end;

destructor TUpdaterMonitor.Destroy;
//var
//  aUpdater: TUpdater;
begin
  FDynamicAttach.Clear;
  FDynamicDetach.Clear;
  Observers.Clear; //My attached observes are all of type IUpdater so are cleaned up automatically because they are reference counted.
//  for aUpdater in Observers do
//    aUpdater.free;
  inherited;
end;

procedure TUpdaterMonitor.Attach(const observer: IUpdater);
begin
  fLock.BeginWrite;
  try
    FDynamicAttach.Add(observer);
  finally
    fLock.EndWrite;
  end;
end;

procedure TUpdaterMonitor.Detach(const observer: IUpdater);
begin
  fLock.BeginWrite;
  try
    FDynamicDetach.Add(observer);
  finally
    fLock.EndWrite;
  end;
end;

procedure TUpdaterMonitor.Notify;
begin
  fLock.BeginWrite;
  try
    if not FDynamicAttach.IsEmpty then
    begin
      FDynamicAttach.ExtractAll(function(const lObserver: IUpdater): Boolean
                                begin
                                  inherited Attach(lObserver);
                                end);
    end;

    if not FDynamicDetach.IsEmpty then
    begin
      FDynamicDetach.ExtractAll(function(const lObserver: IUpdater): Boolean
                                begin
                                  inherited Detach(lObserver);
                                end);
    end;
    if not FDynamicAttach.IsEmpty or
       not FDynamicDetach.IsEmpty then
      writeln('not empty');
  finally
    fLock.EndWrite;
  end;

  inherited Notify;
end;

procedure TUpdaterMonitor.DoNotify(const observer: IUpdater);
begin
  observer.DapInput := FDapInput;
  observer.Update(FID);
end;

Your idea of dumping the list of observers to an array is slick and works perfectly. My code now looks like this, and works perfectly.

    TUpdaterMonitor = class(TObservable<IUpdater>)
    private
      FDapInput : TArray<smallint>;
      FID     : integer;
    public
      destructor Destroy; override;
      procedure DoNotify(const observer: IUpdater); override;
      procedure Notify;
      property DapInput: TArray<smallint> write FDapInput;
      property ID: integer write FID;
    end;

destructor TUpdaterMonitor.Destroy;
//var
//  aUpdater: TUpdater;
begin
  Observers.Clear; //My attached observes are all of type IUpdater so are cleaned up automatically because they are reference counted.
//  for aUpdater in Observers do
//    aUpdater.free;
  inherited;
end;

procedure TUpdaterMonitor.Notify;
var
  observer: IUpdater;
  observerArray: TArray<IUpdater>;
begin
  fLock.BeginRead;
  try
    observerArray := fObservers.ToArray;
    for observer in observerArray do
      DoNotify(observer);
  finally
    fLock.EndRead;
  end;
end;

procedure TUpdaterMonitor.DoNotify(const observer: IUpdater);
begin
  observer.DapInput := FDapInput;
  observer.Update(FID);
end;

So now I am wondering should TObserver's Notify procedure be updated to dump the list to an array? At least in my case it does the trick. I could alter my code to be careful not to ever have an observer essentially instantiate and attach another observer. That seems, to me, an easy problem that could re-occur easily. What do you think?

Thanks again for getting back to me. ToArray is an elegant solution.

Ryan