spinettaro / delphi-event-bus

Delphi Event Bus (for short DEB) is an Event Bus framework for Delphi
Apache License 2.0
459 stars 105 forks source link

Error invoking subscriber method. Subscriber class: <something> . Original exception: EInvalidCast: Invalid class typecast #47

Closed esseredigitale closed 3 years ago

esseredigitale commented 3 years ago

Hi, I use a MQTTClient and onReceive I post a event for a lot of subscriber on my code.

procedure TDMMain.riempiDatiMqtt(ADevice, APayLoad : string);
var
  LEvent: IMQTTReceiveEvent;
begin
  try
      LEvent := GetMQTTReceiveEvent;
      LEvent.SN := ADevice;
      LEvent.Payload := APayLoad;
      ToCodeSite('LEvent',LEvent.SN + ' ' + LEvent.Payload);
      try
        GlobalEventBus.Post(LEvent);
      except
        on e : exception do
        begin
          ToCodeSite('GlobalEventBus.Post(LEvent) except: ', e.Message);
        end;
      end;
  except
    on e: Exception do
    begin
      ToCodeSite('RiempiDatiMqtt except: ', e.Message);
    end;
  end;
end;

Rarely, the GlobalEventBus.Post method raises a typecast error like subject with different type of Subscriber class. like TAniindicator, TStyledEdit, TEdit, and more.

Can you help me?

wxinix commented 3 years ago

This is the exception thrown from INSIDE your subscriber method. You probably need to check the code of your subscriber method.

esseredigitale commented 3 years ago

Hi, but it's very simple:

[Subscribe(TThreadMode.Main)]
procedure NewUpdateStatus(AEvent: IMQTTReceiveEvent);

[...]

procedure TfrDevice.NewUpdateStatus(AEvent: IMQTTReceiveEvent);
var
  _payload, _temp, _Umid, _pump: string;
begin
  try
    if AEvent.SN = SN then
    begin
      Mostra;
      DMMain.ToCodeSite('Update Status From Device', SN + ': Sto scompattando il pacchetto');
      _payload := AEvent.Payload;
      _temp := copy(_payload,2,5);
      _Umid := copy(_payload,7,5);
      _pump  := copy(_payload,12,5);
      Temperatura  := StrToInt(_temp);
      Umidita      := StrToInt(_Umid);
      Pompa        := StrToInt(_ozo);
      Pompa2 := StrToInt(_payload[17]);
      Ventola      := StrToInt(_payload[18]);
    end;
  Except
    on e : exception do
    begin
      DMMain.ToCodeSite('New Update Status from Device except: ', e.Message);
    end;
  end;
end;
spinettaro commented 3 years ago

@esseredigitale hi Alfredo, you can easily put a log after the begin of the procedure TfrDevice.NewUpdateStatus(AEvent: IMQTTReceiveEvent); . If you see that log, as @wxinix was saying, it's not related to DEB

esseredigitale commented 3 years ago

@spinettaro Hi Daniele, I discovered DEB during your presentation at Andrea Magni's Delphi Live! We have emptied the method but the exception is always raised. Could it be useful to know that the subscriber is a frame?

wxinix commented 3 years ago

Are IMQTTReceiveEvent declaration and its implementation in different packages?

Is Runtime with Packages enabled in project settings?

wxinix commented 3 years ago

The only possibility that this exception raised is where the line [AEvent as TObject]

Then I suspect your IMQTTReceiveEvent implementation is in a runtime Package. Is that the case?

Besides why not you follow the debugger step-into to see which line of code throw the exception? That is the whole point of open-source, and DEB source is not that convoluted at all for you to trace.

esseredigitale commented 3 years ago
unit uDEB;

interface

type
  IMQTTReceiveEvent = interface
  ['{4B8A9025-AEF0-4FDA-AEDC-A001F998AE3B}']
    procedure SetSN(const Value: string);
    function GetSN: string;
    property SN: string read GetSN write SetSN;
    procedure SetPayload(const Value: string);
    function GetPayload: string;
    property Payload: string read GetPayload write SetPayload;
  end;

  function GetMQTTReceiveEvent: IMQTTReceiveEvent;

implementation

type
  TMQTTReceiveEvent = class(TInterfacedObject, IMQTTReceiveEvent)
  private
    FPayload: string;
    FSN: string;
    procedure SetPayload(const Value: string);
    function GetPayload: String;
    procedure SetSN(const Value: string);
    function GetSN: String;
  public
    property Payload: string read GetPayload write SetPayload;
    property SN: string read GetSN write SetSN;
  end;

{ TMQTTReceiveEvent }

function TMQTTReceiveEvent.GetPayload: String;
begin
  result := FPayload;
end;

function TMQTTReceiveEvent.GetSN: String;
begin
  result := FSN
end;

procedure TMQTTReceiveEvent.SetPayload(const Value: string);
begin
  FPayload := Value;
end;

procedure TMQTTReceiveEvent.SetSN(const Value: string);
begin
  FSN := Value;
end;

function GetMQTTReceiveEvent: IMQTTReceiveEvent;
begin
  result := TMQTTReceiveEvent.Create;
end;

end.

No runtime packages, only an EXE. Following debugger, puttin a codesite message

procedure TEventBus.InvokeSubscriber(ASubscription: TSubscription; const AEvent: IInterface);
begin
  try
    if not ASubscription.Active then
      Exit;
    CodeSite.Send(ASubscription.Subscriber.ClassName);

    ASubscription.SubscriberMethod.Method.Invoke(ASubscription.Subscriber,  [AEvent as TObject]);
  except
    on E: Exception do begin
      raise Exception.CreateFmt(
        'Error invoking subscriber method. Subscriber class: %s. Event type: %s. Original exception: %s: %s',
        [
          ASubscription.Subscriber.ClassName,
          ASubscription.SubscriberMethod.EventType,
          E.ClassName,
          E.Message
        ]);
    end;
  end;
end;

we receive TFMain, TfrDevice, but at a certain point, we got TCheckBox, TActiveStyleTextObject or more, with exception

spinettaro commented 3 years ago

Hi @esseredigitale , are you able to create a little project that highlights the issue and share it with us? That would be great ! so we can investigate on a real project/code

esseredigitale commented 3 years ago

@wxinix using your fork we had same error. @spinettaro we are modifying the source to eliminate the DB and access to the MQTT server, so you can have a demo as similar as possible to the real project

esseredigitale commented 3 years ago

Sure, we receive TStyleEdit, TCheckBox, TActiveStyleTextObject or more at ASubscription.Subscriber.ClassName raising "Error invoking subscriber method. Subscriber class: . Original exception: EInvalidCast: Invalid class typecast ... etc.

esseredigitale commented 3 years ago

@esseredigitale @spinettaro

Did you use TStyleEdit, TCheckBox, TActiveStyleTextObject as the Subscriber Object, thus having the Form class as their Parent?

we are making a demo without db and other components ... For now I can tell you that the project is made with a DataModule with a component MQTT client that receives messages from a server and for each message we do a globalevent post.

procedure TDMMain.TMSMQTTClient1PublishReceived(ASender: TObject; APacketID: Word; ATopic: string; APayload: TArray<System.Byte>);
var
  _pl: string;
  _devicename, _topic : string;
  LEvent: IMQTTReceiveEvent;
begin
  _pl := TEncoding.UTF8.GetString(APayload);
  _devicename := copy(ATopic,0,pos('/',ATopic) - 1);
  _topic := Copy(ATopic,pos('/',ATopic),length(ATopic)-length(_devicename));
  if _topic = STATUS then
  begin
    LEvent := GetMQTTReceiveEvent;
    LEvent.SN := _devicename;
    LEvent.Payload := _pl;
    ToCodeSite('LEvent',LEvent.SN + ' ' + LEvent.Payload);
    try
      GlobalEventBus.Post(LEvent);
    except
      on e : exception do
      begin
        ToCodeSite('GlobalEventBus.Post(LEvent) except: ', e.Message);
      end;
    end;
  end;
end;

Subscriber are in many frames on a TGridLayout on the main form.

[Subscribe(TThreadMode.Main)]
procedure NewUpdateStatus(AEvent: IMQTTReceiveEvent);
[...]
procedure TfrOzonum.NewUpdateStatus(AEvent: IMQTTReceiveEvent);
var
  _payload, _temp, _Umid, _ozo: string;
begin
  try
    if AEvent.SN = SN then
    begin
      Mostra;
      DMMain.ToCodeSite('Update Status From Ozonum', SN + ': Sto scompattando il pacchetto');
      _payload := AEvent.Payload;
      _temp := copy(_payload,2,5);
      _Umid := copy(_payload,7,5);
      _ozo  := copy(_payload,12,5);
      Temperatura  := StrToInt(_temp);
      Umidita      := StrToInt(_Umid);
      Ozono        := StrToInt(_ozo);
      Ozonizzatore := StrToInt(_payload[17]);
      Ventola      := StrToInt(_payload[18]);
    end;
  Except
    on e : exception do
    begin
      DMMain.ToCodeSite('New Update Status from Ozonum except: ', e.Message);
    end;
  end;
end;

What do you mean when write "Subscriber Object"?

If so, then that is THE PROBLEM.

If you look into System.Rtti for the source of the Rtti Invoke code (as in the Figure) - you will see that an EInvalidCast exception will be thrown, if the Subscriber Object is NOT of a derived type of its PARENT.

This explain why TForm, TFrame, TDateModule works, while TStyleEdit, TCheckBox, TActiveStyleTextObject NOT.

So basically, the conclusion is that: we cannot have child components dropped to a Form as the Subscribe Object., at all.

2020-12-02_18-47-15

TStyleEdit and TActiveStyleTextObject are used in the TStylebook of the main form.

esseredigitale commented 3 years ago

Is the subscriber object, i.e the object on which the subscriber method is defined, a visual component dropped on some other visual component (like a Form, a Frame, Layout etc)? For example, if you have an Edit called Edit1, inside a Frame called Frame1, and you register it RegisterSubscriberForEvents(Edit1) —— this won’t work because Edit1.Parent = Frame1, and Edit1 does NOT inherit from Frame1. System.RTTI would throw the EInvalidCast exception in this case when the EventBus trying to invoke the subscriber method via RTTI. In your code, procedure TfrOzonum.NewUpdateStatus, what type is TfrOzonum? Is it a Frame INDIDE another TGridLayout component and having the TGridLayout as its Parent? That would cause this myopic EInvalidCast, Frame class does not inherit from GridLayout class. A workaround would be to put Frame A inside another Frame B, put Frame B inside the TGridLayout, then Register Frame A to the EventBus to circumvent the Exception condition.

TfrOzonum is a Frame and his Parent is a TGridLayout on main form. On main form there is a method who fills the TGridLayout and after each frame creation, calls a public method of the frame:

procedure TfrOzonum.DoShow;
begin
  try
    GlobalEventBus.RegisterSubscriberForEvents(self); // MY MISTAKE HERE???
    TTask.Run(
      procedure
      begin
        repeat
          if DMMain.UnsubscribeIt then
            GlobalEventBus.UnregisterForEvents(Self);
        until DMMain.UnsubscribeIt
      end
    );
  except
    on e : Exception do
    begin
      DMMain.ToCodeSite('DoShow from Ozonum except',e.Message);
    end;
  end;
end;
wxinix commented 3 years ago

TfrOzonum is a Frame and his Parent is a TGridLayout on main form.”

So pretty much that is the cause of the problem

The Solution is simple: @esseredigitale

- Just fit TfrOzonum inside another extra frame, then put this other frame to TGridLayout, like this structure: TfrOzonum —>another frame —> GridLayout

This way, TfrOzonum’s Parent is a Frame and itself is also a Frame. This will get rid of the EInvalidCast exception, thrown by System.Rtti

The lessons learned Subscriber Object MUST BE a derived type of its PARENT type, typically this is true, but may be false when it comes to using VCL Components as the Subscriber (like using a Frame as the Subscriber, which lies inside a parent GridLayout) If a VCL component's Parent (its container object) is of a DIFFERENT type from the VCL component itself, then DelphiEventBus won't work and the system will thrown EInvalidCast exception

esseredigitale commented 3 years ago

Sorry to inform you that with this method program raise same error! Our project is a firemonkey project, are there any differences?

wxinix commented 3 years ago

@esseredigitale Sorry - my theory is wrong. And sorry for all my wild guesses.

I guess the best is to post your demo project that others can replicate the issue.

esseredigitale commented 3 years ago

Don't get down! We will find the problem

esseredigitale commented 3 years ago

Hi, you can download source of demo from https://1drv.ms/u/s!Aj-4lQEtPyPlha8XsHOvy-o0WQFJXQ?e=f3k7KW Use procedure:

  1. select an item from first combobox
  2. select an item from second combobox
  3. press "Visualizza"
  4. after some seconds, press first combobox again and ... boom!
spinettaro commented 3 years ago

@esseredigitale the link seems broken or it is pointing to something bad https://github.com/spinettaro/delphi-event-bus/issues/url

esseredigitale commented 3 years ago

sorry but GitHub modify url https://1drv.ms/u/s!Aj-4lQEtPyPlha8Xp4Xy41jANVFIlA?e=zKAeAW copy and paste it on browser

wxinix commented 3 years ago

@esseredigitale The tricky point is at this line:

 Main:
      if (AIsMainThread) then
        InvokeSubscriber(ASubscription, AEvent)
      else
        TThread.Queue(nil, GenerateThreadProc(ASubscription, AEvent)); //  <<---  ASubscription is captured here
    Background:

When you are posting to MainThread from a different worker thread, ASubscription is Captured in the anonymous method, but the anonymous method will be invoked at a later time by the MainThread. If in between the time, the suscription is UnRegistered (again, by a separate thread), the ASubscription reference captured by the anonymous method will point to invalid memory address (hence you see randomly all kinds of "phantom subscriber" like StyleEdit, ItemList, ScrollBar due to the invalid ASubsription reference).

Then when the MainThread starts to invoke the anonymous method with the invalid ASubscription, System.Rtti.Invoke will throw the exception at the following line (because the unmatched Subscriber object, and the method member):

    if (cls <> nil) and not cls.InheritsFrom(TRttiInstanceType(Parent).MetaclassType) then
      raise EInvalidCast.CreateRes(@SInvalidCast);

@spinettaro What do you think? The cause of the problem is that the ASubscription captured by the anoymous method become obsolete by another thread unRegistering the subscriber, but the MainThread doesn't know about that and still tries to invoke the anoymous method with the invalid ASubscription.

We need to figure out a way so that when a Subscription is deleted (unregistered) in a different thread context, any earlier capture of that Subscription in anonymous methods can be handled properly .... ?

esseredigitale commented 3 years ago

I tried to remove explicit unregistering, unfortunely with same result

wxinix commented 3 years ago

I tried to remove explicit unregistering, unfortunely with same result

That is a different problem on your application layer, not on DEB layer. You might want to make sure when changing the Combox the subscriber object itself is still valid (I saw the Frames are deleted?)

esseredigitale commented 3 years ago

GeneraValori is only for demo: we have a MQTT client provided by TMS, for receiving the data from a internet broker. So, we haven't to unregister before destroy a frame? I tried unfortunely with same error

spinettaro commented 3 years ago

@esseredigitale @wxinix it's weird... the problem is that somehow a Subscriber in a specific moment in time is something different from TfrOzonum: one time it is a TLabel, another time a TActiveStyleTextObject and so on... That causes the exception, I'm investigating on the reason.... @wxinix any idea? How the Subscriber can change? I have also put a breakpoint to understand if we are storing a wrong class, but it seems we are storing a right value...

spinettaro commented 3 years ago

ok the fix is to put this:

destructor TfrOzonum.Destroy;
begin
  GlobalEventBus.UnregisterForEvents(Self);
  inherited;
end;

@esseredigitale the code you written to unregister the frame is quite weird. My suggestion is to register the frame on the DoShow and unregister it on the destroy. @esseredigitale what do you think?

@wxinix that is because somehow during the destructor the memory address points to something else.

wxinix commented 3 years ago

@spinettaro @esseredigitale

Subscriber in a specific moment in time is something strange: once TLabel, another time a TActiveStyleTextObject and so on... T

Regardless of anything with this specific demo project, there is a potential bug caveat with DEB: As seen in the code below: DEB using anonymous method here, pushing the anonymous method to TThread.Queue. ASubscription (which encapsulates the Subscriber Object, and the associated Subscriber Method) is captured by that anonymous method but NOT immediately executed.

Then, from a different thread, if the subscriber is deleted (like when we switch to a different Combox item in this example), then the captured Subscription reference would points to invalid subscriber, if Unregister is not property called.

 Main:
      if (AIsMainThread) then
        InvokeSubscriber(ASubscription, AEvent)
      else
        TThread.Queue(nil, GenerateThreadProc(ASubscription, AEvent)); //  <<---  ASubscription is captured here
    Background:

The following code fixes for the demo project, because the destructor is called synchronously by the MainThread.

destructor TfrOzonum.Destroy;
begin
  GlobalEventBus.UnregisterForEvents(Self);
  inherited;
end;
esseredigitale commented 3 years ago

How can I implement a destructor for a frame?

wxinix commented 3 years ago

How can I implement a destructor for a frame?

Like this?

destructor TfrOzonum.Destroy;
begin
  GlobalEventBus.UnregisterForEvents(Self);
  inherited;
end;
esseredigitale commented 3 years ago

How can I implement a destructor for a frame?

Like this?

destructor TfrOzonum.Destroy;
begin
  GlobalEventBus.UnregisterForEvents(Self);
  inherited;
end;

Yes I know .. but I forgotten override ... sorry.

It works!!! Great, thanks

spinettaro commented 3 years ago

@wxinix there is the active check for that before the invoking:

if not ASubscription.Active then
      Exit;

that is set to false when we unregister a subscriber. In that case we are not going to call the Subscriber because the subscription is not active anymore.

ASubscription is something internal. The only scenario is when a developer delete a Subscriber but forget to unregister it. But in that case it's a developer mistake, we cannot do too much from a framework point of view.

We could add an additional check to understand the Subscriber is still assigned, but may be a false positive because it could be a wild reference

@wxinix what do you think?

wxinix commented 3 years ago

@spinettaro

I think what you said makes sense

It is the user’s responsibility to unregister a subscription.

Extra care should be taken to make sure that the unregistration of a subscriber and destroy of that same subscriber should be performed SYNCHRONOUSLY.

Thank you again for this open source project and thank Alfredo for his demo project. I get much better/deep understanding of the entire work and related, now feel very confident using DEB in projects now.

esseredigitale commented 3 years ago

@spinettaro @wxinix thank for your job and for the support! I'm glad I was helpful. In the next version of my software I will insert an about section and have the pleasure to mention you.

spinettaro commented 3 years ago

@esseredigitale I'm glad you are happy and the code finally works. Last advice is to make the code a little bit more "multithreaded". @wxinix thanks for the support :)