TEventBase.Destroy could avoid calling fNotificationHandler.Free when fNotificationHandler is nil

Following issue ~~#274~~, the TEventBase.Destroy code inside Spring.Events.Base was changed to only call fNotificationHandler.Free if UseFreeNotification is true

This means that the test is always true by default because fNotificationHandler value is 0 when the instance is created and GetUseFreeNotification considers 0 as meaning “True but not yet initialized”.

As indicated in the above issue, we have a local change where the call to fNotificationHandler.Free is surrounded by a call to TThread.Synchronize and with the current test, that call is always made but most times is of no use because (0).Free will be a no-op.

During the past 4 years, we did not notice any adverse effect of that situation and were quite happy with the following local code:

  if UseFreeNotification then
  begin
    TThread.Synchronize(nil,
      procedure
      begin
        fNotificationHandler.Free;
      end);
  end;
  NativeInt(fNotificationHandler) := 0;

As our use cases evolved, we are now using existing code in the context of a DLL that is loaded by a third party program where we are faced with a never ending call to FreeLibrary that tries to unload our DLL.

We tracked that issue to the fact that a TEvent instance is being destroyed during finalization, thus deadlocking inside TThread.Synchronize which is quite logical. But in our case, the event is not attached to any TComponent derived class, so fNotificationHandler stays 0 for all the lifetime of the event object.

This is why we believe it would be appropriate to change the UseFreeNotification test so that it is True only if the handler was ever instantiated, and we now have this test:

  if NativeInt(fNotificationHandler) > 1 then

At first we simply added and Assigned(fNotificationHandler) to the existing test but we believe the version above makes more sense because it uses less instructions and makes it clearer that the special 1 value is taken into account.

‌

We understand that our local change is the source of the issue and are not blaming you for anything here. It would however be nice if our suggested change be implemented in Spring, in part because we believe it means fNotificationHandler.Free is now never called when there is no need for it to be called.

‌

Thanks for your work and the time you take to consider our suggestion.

Comments (4)