SchneiderInfosystems / FB4D

Open Source Library for connecting Firebase by Delphi VCL/FMX
https://www.schneider-infosys.ch/DelphiBlog/de/#FB4D
Apache License 2.0
180 stars 53 forks source link

Access violation using more than one listener #143

Closed PixelPlanet-MH closed 2 years ago

PixelPlanet-MH commented 2 years ago

Hello,

I've tried to use two firebase listeners (on a windows system) created with ListenForValueEvents.

Starting and stopping the listeners multiple times result in an access violation on fstream used in class TRTDBListenerThread.

It seems the problem is with fGetFinishedEvent. The Event ist created for each listener thread with the same name 'RTDBListenerGetFini'. But Events with the same name are equal (although they are different objects and have different handles).

And so fGetFinishedEvent.SetEvent in TRTDBListenerThread.OnEndListenerGet for one listener thread can signal the fGetFinishedEvent.WaitFor in Execute method for another thread, which I think is not intended.

I changed to use EventName with '' in TEvent.Create(nil, false, false, EventName), so different threads are using different events.

And then starting and stopping the listeners multiple times did not result in access violations.

with kind regards Michael

SchneiderInfosystems commented 2 years ago

Hi Michael

Thank you for using FB4D and for your message. I have immediately seen your issue report on git. Do you work on the newest version?

I need probably some time to analyse this issue. If you have a proposal to fix let me know.

Kind regards

Christoph

Von: PixelPlanet-MH @.> Gesendet: Thursday, 22 September 2022 16:24 An: SchneiderInfosystems/FB4D @.> Cc: Subscribed @.***> Betreff: [SchneiderInfosystems/FB4D] Access violation using more than one listener (Issue #143)

Hello,

I've tried to use two firebase listeners (on a windows system) created with ListenForValueEvents.

Starting and stopping the listeners multiple times result in an access violation on fstream used in class TRTDBListenerThread.

It seems the problem is with fGetFinishedEvent. The Event ist created for each listener thread with the same name 'RTDBListenerGetFini'. But Events with the same name are equal (although they are different objects and have different handles).

And so fGetFinishedEvent.SetEvent in TRTDBListenerThread.OnEndListenerGet for one listener thread can signal the fGetFinishedEvent.WaitFor in Execute method for another thread, which I think is not intended.

I changed to use EventName with '' in TEvent.Create(nil, false, false, EventName), so different threads are using different events.

And then starting and stopping the listeners multiple times did not result in access violations.

with kind regards Michael

— Reply to this email directly, view it on GitHub https://github.com/SchneiderInfosystems/FB4D/issues/143 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ACHGKTZ5I5MJMFKLEKLM5FLV7RTYXANCNFSM6AAAAAAQTDP2AM . You are receiving this because you are subscribed to this thread. https://github.com/notifications/beacon/ACHGKT4OHFH2AFG3MMOAGYLV7RTYXA5CNFSM6AAAAAAQTDP2AOWGG33NNVSW45C7OR4XAZNFJFZXG5LFVJRW63LNMVXHIX3JMTHFEZ7TRI.gif Message ID: @. @.> >

PixelPlanet-MH commented 2 years ago

Hello Christoph

We are using the latest version 1.3.1

Kind regards Michael

Von: Christoph Schneider @.> Gesendet: Donnerstag, 22. September 2022 16:43 An: SchneiderInfosystems/FB4D @.> Cc: Michael Hinrichs @.>; Author @.> Betreff: Re: [SchneiderInfosystems/FB4D] Access violation using more than one listener (Issue #143)

Hi Michael

Thank you for using FB4D and for your message. I have immediately seen your issue report on git. Do you work on the newest version?

I need probably some time to analyse this issue. If you have a proposal to fix let me know.

Kind regards

Christoph

Von: PixelPlanet-MH @.> Gesendet: Thursday, 22 September 2022 16:24 An: SchneiderInfosystems/FB4D @.> Cc: Subscribed @.***> Betreff: [SchneiderInfosystems/FB4D] Access violation using more than one listener (Issue #143)

Hello,

I've tried to use two firebase listeners (on a windows system) created with ListenForValueEvents.

Starting and stopping the listeners multiple times result in an access violation on fstream used in class TRTDBListenerThread.

It seems the problem is with fGetFinishedEvent. The Event ist created for each listener thread with the same name 'RTDBListenerGetFini'. But Events with the same name are equal (although they are different objects and have different handles).

And so fGetFinishedEvent.SetEvent in TRTDBListenerThread.OnEndListenerGet for one listener thread can signal the fGetFinishedEvent.WaitFor in Execute method for another thread, which I think is not intended.

I changed to use EventName with '' in TEvent.Create(nil, false, false, EventName), so different threads are using different events.

And then starting and stopping the listeners multiple times did not result in access violations.

with kind regards Michael

— Reply to this email directly, view it on GitHub https://github.com/SchneiderInfosystems/FB4D/issues/143 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ACHGKTZ5I5MJMFKLEKLM5FLV7RTYXANCNFSM6AAAAAAQTDP2AM . You are receiving this because you are subscribed to this thread. https://github.com/notifications/beacon/ACHGKT4OHFH2AFG3MMOAGYLV7RTYXA5CNFSM6AAAAAAQTDP2AOWGG33NNVSW45C7OR4XAZNFJFZXG5LFVJRW63LNMVXHIX3JMTHFEZ7TRI.gif Message ID: @. @.> >

— Reply to this email directly, view it on GitHubhttps://github.com/SchneiderInfosystems/FB4D/issues/143#issuecomment-1255129930, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A3HAAXABPF4EJ5MELYKRMTLV7RV7JANCNFSM6AAAAAAQTDP2AM. You are receiving this because you authored the thread.Message ID: @.***>

SchneiderInfosystems commented 2 years ago

I have analyzed the registration of a 2nd listener. If I implement everything correctly, two independent listeners are working properly.

image

What is essential: You need for the 2nd listener an own IFirebaseEvent variable. If you overwrite the first used IFirebaseEvent then the reader thread will be immediately stopped as a consequence of freeing the former IFirebaseEvent. This could lead to an access violation in your code.

I have added only the following code to FB4D.DemoFmx.pas in order to test your scenario:

fFirebaseEvent2: IFirebaseEvent;

procedure TfmxFirebaseDemo.btnStart2ndListenerClick(Sender: TObject);
begin
  if not CheckAndCreateRealTimeDBClass(memScans) then
    exit;
  fFirebaseEvent2 := fRealTimeDB.ListenForValueEvents(
    SplitString(edtRTDBEventPath.Text.Replace('\', '/'), '/'),
    OnRecData2, OnRecDataStop2, OnRecDataError2);
  if assigned(fFirebaseEvent2) then
    memScans.Lines.Add(TimeToStr(now) + ': 2nd Event handler started for ' +
      TFirebaseHelpers.ArrStrToCommaStr(fFirebaseEvent2.GetResourceParams))
  else
    memScans.Lines.Add(TimeToStr(now) + ': end Event handler start failed');
  btnStart2ndListener.Enabled := false;
  btnStopEvent2.Enabled := true;
end;

procedure TfmxFirebaseDemo.btnStopEvent2Click(Sender: TObject);
begin
  if assigned(fFirebaseEvent2) then
    fFirebaseEvent2.StopListening;
end;

procedure TfmxFirebaseDemo.OnRecData2(const Event: string;
  Params: TRequestResourceParam; JSONObj: TJSONObject);
var
  par, p: string;
begin
  par := '[';
  for p in Params do
  begin
    if par.Length > 1 then
      par := par + ', ' + p
    else
      par := par + p;
  end;
  memScans.Lines.Add(TimeToStr(now) + '::2nd: ' + Event + par + '] = ' +
    JSONObj.ToJSON);
end;

procedure TfmxFirebaseDemo.OnRecDataError2(const Info, ErrMsg: string);
begin
  memScans.Lines.Add(TimeToStr(now) + ':: 2nd Error in ' + Info + ': ' + ErrMsg);
end;

procedure TfmxFirebaseDemo.OnRecDataStop2(Sender: TObject);
begin
  memScans.Lines.Add(TimeToStr(now) + ': 2nd Event handler stopped');
  fFirebaseEvent2 := nil;
  btnStart2ndListener.Enabled := true;
  btnStopEvent2.Enabled := false;
end;
SchneiderInfosystems commented 2 years ago

BTW: I found during the investigation a small cosmetic failure but this had no impact on the functionality of the listener

PixelPlanet-MH commented 2 years ago

Hello Christoph,

sorry, I should have been more precise. So I have used two different variables for the two listeners and the listeners are listening on different locations.

So I think the problem is with the Event and I wrote some minimal example which I think shows it.

In this example there are two threads which correspond to the listener threads (TRTDBListenerThread). Each thread has an event which corresponds to fGetFinishedEvent.

So in this example the event in the first thread is signaled, while the event in the second thread is not. And so the exception in the second thread should never be raised. But it is raised (at least on a windows system) because the name used for the event is the same (RTDBListenerGetFini).

And so in case of your listener threads the fstream object is freed when fGetFinishedEvent is signaled (by another listener thread) which can lead to access violation because the http connection has not finished and will further access the fstream.

` TThread.CreateAnonymousThread( procedure var FEvent1 : TEvent; EventName : string; begin EventName := 'RTDBListenerGetFini'; FEvent1 := TEvent.Create(nil, false, false, EventName);

  while not TThread.CheckTerminated do
  begin
    FEvent1.SetEvent;
    sleep(200);
    Fevent1.WaitFor();
  end;

  FEvent1.Free;
end

).Start;

TThread.CreateAnonymousThread( procedure var FEvent2 : TEvent; EventName : string; begin EventName := 'RTDBListenerGetFini'; FEvent2 := TEvent.Create(nil, false, false, EventName); FEvent2.WaitFor(); raise Exception.Create('This should never be raised'); FEvent2.Free; end ).Start;`

My Fix to this problem was to use an empty name for the event ( EventName := ''). I hope this can help with kind regards Michael

SchneiderInfosystems commented 2 years ago

Please check the new commit. I have added that the event name depends on the number of installed listeners. To demonstrate 2 concurrent running listeners, I have extended the Intro Demo.

PixelPlanet-MH commented 2 years ago

Hello Christoph,

I have checked the new commit and it works. Thank you for the fix!

with kind regards Michael