TEventBase should synchronize calls related to FreeNotification

[Olivier Sannier]

Let's consider the following architecture :

Upon startup, the main application initializes a factory for IMyInterface by giving it a set of events (procedure of object) that are to be given to any instance the factory will create. Some of those events are located inside an instance of a class derived from TComponent

During the life of the application, many calls are done to the factory to get a new instance of IMyInterface using an implementation that stores the events inside IEvent<T> instances. Most of those calls are done inside secondary threads and thus happen simultaneously.

The instances for IMyInterface are local to the thread, that is they are released before the thread ends, which means the destructor is called in the context of that thread.

Now, if you look at TEventBase.Notify you'll see that the event registers its fNotificationHandler object into the FreeNotifiers of the object that holds the handler to be called because the object inherits from TComponent

Similarly, if you look at TEventBase.Destroy, the destruction of fNotificationHandler will ultimately lead to its removal from the FreeNotifiers list for the object that holds the handler to be called.

The problem is that those two calls are done inside the context of secondary threads, but they call methods (FreeNotification and RemoveFreeNotifications) that are definitely not thread-safe.

What happens in our context is that we get access violations upon destroying the component which handlers were given to the IEvent instances because its FreeNotifiers list is in an inconsistent state.

The most direct solution is to surround the existing call with a TThread.Synchronize(nil, procedure begin ... end) construct which is what is offered in the attached patch.

However, that class method is not available in all supported Delphi versions and I could not find a way to do it for older versions without consuming extra memory for every IEvent instance, which clearly goes against what eb1d623 has achieved.

I agree that this is a corner issue, and the patch we have applied here is satisfactory for us, but if you can come up with a more universal solution, it would be even nicer.

[Stefan Glienke]

First of all TThread.Synchronize is available on all versions that Spring4D supports (Delphi 2010 and up, soon Delphi XE and up) so no problem there.

Anyway I really hesitate to put that deep into the TEventBase class which causes this to run all the time and causes all component notifications to take place on the main thread.

I rather suggest making sure that event attaching and detaching is being put into TThread.Synchronize inside of your code to make sure that it does not run into concurrency issues. Alternatively you can consider using a class that is not TComponent as event handler because its mechanic is not necessary in your case. It is just used to detach event handlers when the handler instance is being destroyed.

[Olivier Sannier]

The calls to Synchronize are made when Adding/Removing to the event (the code to protect is in Notify) and when destroying it (in Destroy), but never when triggering it.

So the only calls that gets run in the main thread are the setup calls, not the usage calls which is why I'm fine with our patch.

You are right that we could add the call to TThread.Synchronize in our code, but I'm afraid that this situation could also arise in other applications that do not use the factory pattern.

I understand what the code is about and I find it clever, but in our case it is of no use and so I also considered that maybe there could be a flag on TEvent that says "handle TComponent" that is trueby default and could be deactivated via an overloaded constructor.

Its storage could be done in bit 30 of fCountjust like what is done for Disabled(bit 31) thus not consuming more memory.

[Stefan Glienke]

Optionally disabling the fNotificationHandler is worth considering - however I would not store that in an fCount bit as that makes then GetCanInvoke and GetEnabled more complicated than necessary. I would store that as some non nil value in the fNotificationHandler field itself.

[Olivier Sannier]

Yes, you are right, that would be much clearer, and having something like 1in fNotificationHandler would clearly mark it as a special value as any valid pointer is aligned on a NativeInt boundary

That solution means a bit more work on our end, but I understand that your code must care for the needs of everyone, so we will adapt when the change is available.

[Stefan Glienke]

Please have a look at b1720758 and let me know if that works for you.

[Stefan Glienke]

@obones Did you have time to look into this? I would like to merge if that is working for you.

[Olivier Sannier]

Sorry for the delay, I somehow did not notice the notification email from last month. I just had a look at your changes and made one comment, but overall they look fine to me.

Thanks again for your help.

[Stefan Glienke]

added UseFreeNotification property on events to optionally disable TComponent tracking and automatic unsubscription when the component is being destroyed (fixes #274)

→ <<cset 687a95c6dabb>>