TObservable observer enumeration issue
I have run into a case with the observer pattern where a new updater is being attached while the list of observers is being enumerated which is triggering an error dialog to appear containing this message:
Collection was modified; enumeration operation may not execute.
The best way I can think to combat this is if TObservable<T>.Attach added new observers to a temp list. When .Notify is finished looping over fObservers it could check the temp list and add any in that list to fObservers. I guess the same thing would need to happen when .Detach is called. The observer to detach added to a different temp list. When .Notify is done it would have to check both "add" and "remove" lists to make the necessary updates to the fObservers list.
Shouldn't fLock.BeginWrite, fLock.BeginRead, etc. protect me from this scenario?
i could make the changes to my update monitor class, but I don't know when .notify is finished to know if I can go ahead and .attach/.detach pending observer updates.
Tips/suggestions are welcome.
I think I may have answered my question: I simply need to make sure .attach/.detach calls ALWAYS precede calls to .Notify...... Instances where I need to "spuriously" attach/detach an observer, I should be doing so to temp lists. Then, prior to calls to .notify, process the temp lists accordingly. Life would be easier [for me] if this could be part of TObservable...
Potential PR material?
Thanks, Ryan
Comments (6)
-
repo owner -
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
-
repo owner - changed status to resolved
call ToArray before iterating observers to avoid list error when attach or detach occurs (fixes issue
#165)→ <<cset dbae43d3054c>>
-
reporter @sglienke thanks for implementing the call to ToArray. Today I was finally able to pull down the update, works as advertised. Thanks again.
Ryan
-
repo owner call ToArray before iterating observers to avoid list error when attach or detach occurs (fixes issue
#165)→ <<cset dbae43d3054c>>
-
repo owner - changed version to 1.2
- Log in to comment
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.