Threadklasse mit Event aktualisiert nicht

[Inspur1]

Greetings!

Ich entwickel in FreePascal eine Threadklasse mit einem Event und einigen Eigenschaften.
Um das Problem besser zu erklären, habe ich das Beispiel kompromiert.

Delphi-Quellcode:

unit uTest;

interface

uses Classes;

type
  TTestThread = class(TThread)
  private
    FTestProperty: string;
    FOnTest: TNotifyEvent;
  protected
    procedure DoTest; virtual;
    procedure Execute; override;
  public
    constructor Create(CreateSuspended: Boolean);
    destructor Destroy; override;
    property TestProperty: string read FTestProperty write FTestProperty;
    property OnTest: TNotifyEvent read FOnTest write FOnTest;
  end;

implementation

constructor TTestThread.Create(CreateSuspended: Boolean);
begin
  inherited Create(CreateSuspended);
  FTestProperty := 'default';
end;

destructor TTestThread.Destroy;
begin
  inherited;
end;

procedure TTestThread.DoTest;
begin
  if not Assigned(FOnTest) then
    FOnTest(self);
end;

procedure TTestThread.Execute;
begin
  DoTest;
end;


end.



// Call

private
  FTest: TTestThread;
  procedure MyTest(Sender: TObject);

procedure TForm1.MyTest(Sender: TObject);
begin
  FTest.TestProperty := '>>> Test2 <<<';
end;

procedure TForm1.Button1Click(Sender: TObject);
begin
  FTest := TTestThread.Create(false);
  FTest.FreeOnTerminate := false;
  FTest.OnTest := MyTest;
  FTest.Resume;
  Caption := FTest.TestProperty;
end;

procedure TForm1.FormDestroy(Sender: TObject);
begin
  if Assigned(FTest) then
    FTest.Free;
end;

Ich will im Event die Eigenschaft ändern aber es bleibt bei "default".
Wie mache ich das richtig?

[jaenicke]

Du liest die Eigenschaft direkt nach dem Resume aus. Ein Thread läuft aber eben parallel. Daher wird das Event erst danach ausgeführt.

Dass das Event ausgeführt wird, würdest du mit einem Haltepunkt sofort sehen.

Eine Aktualisierung müsste daher auch dort im Event angestoßen werden, was in dem Beispiel natürlich wenig Sinn macht. Die Konstruktion macht eher Sinn, wenn der Thread den Wert setzt und im Event dann die Aktualisierung angestoßen wird.

Was möchtest du denn erreichen?

[Olli73]

Ein "Not" zu viel:

Delphi-Quellcode:

if not Assigned(FOnTest) then
    FOnTest(self);

[Inspur1]

Ich möchte mehrere HTML auslesen, im Event möchte ich festlegen wonach gesucht wird und die Ergebnisse den Eigenschaften übergeben.

[Rollo62]

Du nutzt FTestProperty innerhalb des Threads und außerhalb im UI Thread, ohne irgendeine Art der Synchronisation,
wenn ich das richtig sehe.

Auf jeden Fall sollte man den Zugriff im UI Thread dann synchronisieren, aber die Frage siehe oben, was willst Du erreichen?

[Blup]

vorgeschlagene Änderungen:

Delphi-Quellcode:

uses Classes, System.SyncObjs;

type
  TTestThread = class(TThread)
  private
    FTestProperty: string;
    FOnTest: TNotifyEvent;
    function GetTestProperty: string;
    procedure SetTestProperty(const AValue: string);
  protected
    FCS: TCriticalSection;
    procedure DoTest; virtual;
    procedure Execute; override;
  public
    constructor Create(CreateSuspended: Boolean);
    destructor Destroy; override;
    {kann aus einem anderen Thread gelesen und verändert werden}
    property TestProperty: string read GetTestProperty write SetTestProperty;
    {wird im Hauptthread aufgerufen (ermöglicht Zugriff auf die VCL)}
    property OnTest: TNotifyEvent read FOnTest write FOnTest;
  end;

implementation

procedure TTestThread.DoTest;
begin
  if Assigned(FOnTest) then  // ???: if not Assigned(FOnTest) then
    FOnTest(self);
end;

constructor TTestThread.Create(CreateSuspended: Boolean);
begin
  inherited Create(CreateSuspended);
  FTestProperty := 'default';
  FCS := TCriticalSection.Create;
end;

destructor TTestThread.Destroy;
begin
  FCS.Free;
  inherited;
end;

procedure TTestThread.Execute;
begin
  Synchronize(DoTest);
end;

function TTestThread.GetTestProperty: string;
begin
  FCS.Enter;
  try
    Result := FTestProperty;
  finally
    FCS.Leave;
  end;
end;

procedure TTestThread.SetTestProperty(const AValue: string);
begin
  FCS.Enter;
  try
    FTestProperty := AValue;
  finally
    FCS.Leave;
  end;
end;

procedure TForm1.Button1Click(Sender: TObject);
begin
  FTest := TTestThread.Create(false);
  FTest.FreeOnTerminate := false;
  FTest.OnTest := MyTest;
  FTest.Resume;
  repeat
    CheckSynchronize(10);
  until FTest.Terminated;
  Caption := FTest.TestProperty;
  FTest.Free;
end;

[jaenicke]

Zitat:

Zitat von Olli73 (Beitrag 1540293)

Ein "Not" zu viel:

Delphi-Quellcode:

if not Assigned(FOnTest) then
    FOnTest(self);

Oh ja, das habe ich am Handy glatt übersehen.

[Rollo62]

TL;DR;

Delphi-Quellcode:

procedure TTestThread.Execute;
begin
  Synchronize(DoTest);
end;

Das macht nicht so viel Sinn, denn damit läuft ja dein zu "threadender" Code wieder im Main UI-Thread, wenn ich das richtig interpretiere.
Es sollte doch nur das Ergebnis synchronisiert werden.

[himitsu]

DoTest ist quasi die Methode, welche OnTest auslöst, womit dieses Synchronize schon stimmt. (die Namensgebung ist in der VCL Vielerorts genauso)


Am Ende klingt es so, als wenn der "eigentliche" Code noch viel mehr macht, wie z.B. vorher scheinbar eine Datei runterladen. (natürlich außerhalb des Synchronize)




Prinzipiell sind LongStrings (String/AnsiString/UnicodeString) und ihre Referenzzählung per se thread-save.

Funktional quasi so, als wenn man einen Integer/Boolean/LongBool/Pointer mit den Interlocked-, bzw. Atomic-Funktionen schreibend (Inc/Dec) und kopierend behandelt.

[Rollo62]

Zitat:

Zitat von himitsu (Beitrag 1540325)

DoTest ist quasi die Methode, welche OnTest auslöst, womit dieses Synchronize schon stimmt. (die Namensgebung ist in der VCL Vielerorts genauso)

Delphi-Quellcode:

FTest.OnTest := [B]MyTest[/B];

Das klingt für mich eigentlich danach, dass OnTest die harte Threadarbeit macht ...
Kann mich aber irren

Jedenfalls sieht das hier nicht so aus, als würde der Thread überhaupt irgendwie zum Tragen kommen.
Der Button hängt doch, bis der Thread terminiert ist.

Delphi-Quellcode:

procedure TForm1.Button1Click(Sender: TObject);
begin
  FTest := TTestThread.Create(false);
  FTest.FreeOnTerminate := false;
  FTest.OnTest := MyTest;
  FTest.Resume;
  repeat
    CheckSynchronize(10);
  until FTest.Terminated;
  Caption := FTest.TestProperty;
  FTest.Free;
end;

[himitsu]

TThread.WaitFor anstatt so einer komischen Schleife, falls überhaupt gewartet werden sollte.

[jaenicke]

Alleine schon jemandem zu raten, auf einen Thread zu warten, wenn man gerade mit Threads anfängt, halte ich für kontraproduktiv. Das macht man nicht!

Events sind schon der richtige Weg.

Zitat:

Zitat von Inspur1 (Beitrag 1540294)

Ich möchte mehrere HTML auslesen, im Event möchte ich festlegen wonach gesucht wird und die Ergebnisse den Eigenschaften übergeben.

Warum erst im Event? Brauchst du für die Entscheidung Informationen, die dem Event übergeben werden?

Ansonsten solltest du das dem Thread gleich z.B. im Konstruktor mitgeben, bevor er losläuft.

[himitsu]

PS: Im Button1Click hat es auch niemand gesehn?

Mehrfach klicken,
immer wieder wird die Variable überschrieben
und im Destroy wird dann natürlich nur das Letze freigegeben.

[Inspur1]

Correctly. Das OnTest Event im Beispiel übernimmt die Aufgabe von TThread.Execute. Ich möchte nicht jedes Mal TThread.Execute für jede neue Aufgabe überschreiben, sondern im OnTest Event den Ablauf und die Ergebnisse festlegen und bevor der Thread Free ist die Ergebnisse ausgeben.
Das Problem ist, die Ergebnisse sind nach Änderung noch default.

Delphi-Quellcode:

procedure TTestThread.Execute;
begin
  Synchronize(DoTest);
end;

repeat
  CheckSynchronize(10);
until FTest.Terminated;

Synchronize hat leider nicht geholfen und die Loop blocked die ganze Application.

[Inspur1]

Zitat:

Zitat von jaenicke (Beitrag 1540330)

Warum erst im Event? Brauchst du für die Entscheidung Informationen, die dem Event übergeben werden?

Ansonsten solltest du das dem Thread gleich z.B. im Konstruktor mitgeben, bevor er losläuft.

Right. Die Information soll im Event geholt und bearbeitet werden. Weil verschiedene Inforamtionen von verschiedenen HTML gebraucht werden, muss das Event flexibel sein.

[jaenicke]

Zitat:

Zitat von Inspur1 (Beitrag 1540332)

Correct. Das OnTest Event im Beispiel übernimmt die Aufgabe von TThread.Execute. Ich möchte nicht jedes Mal TThread.Execute für jede neue Aufgabe überschreiben, sondern im OnTest Event den Ablauf und die Ergebnisse festlegen und bevor der Thread Free ist die Ergebnisse ausgeben.

Irgendwie habe ich das Gefühl, dass du eher anonyme Threads brauchst...

TThread ist dafür da, dass du dort die Aktionen implementierst und nur Parameter mitgibst und ggf. Ergebnisse abholst. Deine Beschreibung klingt nun eher danach, dass du bestimmte Funktionen einfach im Thread ausführen möchtest. Das geht dann eher so:

Delphi-Quellcode:

type
  TOnSuccess = reference to procedure(const AResult: string);
  TOnFailure = reference to procedure(const AErrorMessage: string);

function RunDownload(AUrl: string): string;
begin

end;

function Filter(var AResults: string; AFilterParam: string): Boolean;
begin
  Result := True;
end;

procedure ExecuteXyz(AUrl, AParam: string; AOnSuccess: TOnSuccess; AOnFailure: TOnFailure);
begin
  TThread.CreateAnonymousThread(procedure
    var
      Results: string;
    begin
      Results := RunDownload(AUrl);
      if Filter(Results, AParam) then
        TThread.Queue(nil, procedure
          begin
            AOnSuccess(Results);
          end)
      else
        TThread.Queue(nil, procedure
          begin
            AOnFailure('Fehler xyz');
          end)
    end).Start;
end;

procedure TForm273.Button1Click(Sender: TObject);
begin
  ExecuteXyz('http://www.example.com', '',
    procedure(const AResult: string)
    begin
      ShowMessage(AResult);
    end,
    procedure(const AErrorMessage: string)
    begin
      ShowMessage('Fehler:' + sLineBreak + AErrorMessage);
    end);
end;

Ohne mehr Details kann ich aber schlecht konkretere Ratschläge geben. Oft helfen dabei auch Generics weiter, aber das kommt darauf an, welche Aktionen durchgeführt werden und wo diese sich unterscheiden.

[Inspur1]

Das ist ein Teil vom Projekt:

Delphi-Quellcode:

unit uSeriesThread;

interface

uses Classes, Types;

type
  TSeriesThread = class(TThread)
  private
    FTitle: string;
    FUser: string;
    FPwd: string;
    FEpisodes: TStrings;
    FOnHtml: TNotifyEvent;
  protected
    procedure DoHtml; virtual;
    procedure Execute; override;
    procedure SetTitle(Value: string); virtual;
  public
    constructor Create(CreateSuspended: Boolean; OnHtml: TNotifyEvent); overload;
    destructor Destroy; override;
    function GetHtml(Url: string): string;
    procedure Login(User: string; Pwd: string); virtual;
    property Title: string read FTitle write SetTitle;
    property Episodes: TStrings read FEpisodes write FEpisodes;
    property OnHtml: TNotifyEvent read FOnHtml write FOnHtml;
  end;

implementation

//  < API >

type
  HINTERNET = Pointer;
  DWORD_PTR = Cardinal;

const
  WININET_DLL = 'Wininet.dll';
  INTERNET_OPEN_TYPE_PRECONFIG = 0;

function InternetOpen(lpszAgent: PChar; dwAccessType: DWORD;
  lpszProxy, lpszProxyBypass: PChar; dwFlags: DWORD): HINTERNET;
  stdcall; external WININET_DLL name {$IFDEF UNICODE}'InternetOpenW'{$ELSE}'InternetOpenA'{$ENDIF};

function InternetOpenUrl(hInet: HINTERNET; lpszUrl, lpszHeaders: PChar;
  dwHeadersLength, dwFlags: DWORD; dwContext: DWORD_PTR): HINTERNET;
  stdcall; external WININET_DLL name {$IFDEF UNICODE}'InternetOpenUrlW'{$ELSE}'InternetOpenUrlA'{$ENDIF};

function InternetReadFile(hFile: HINTERNET; lpBuffer: Pointer;
  dwNumberOfBytesToRead: DWORD; var lpdwNumberOfBytesRead: DWORD): Boolean;
  stdcall; external WININET_DLL;

function InternetCloseHandle(hInet: HINTERNET): Boolean;
  stdcall; external WININET_DLL;

//  < /API >

//  < TSeriesThread >

constructor TSeriesThread.Create(CreateSuspended: Boolean; OnHtml: TNotifyEvent);
begin
  inherited Create(CreateSuspended);
  FEpisodes := TStringList.Create;
  FOnHtml := OnHtml;
end;

destructor TSeriesThread.Destroy;
begin
  FEpisodes.Free;
  inherited;
end;

function TSeriesThread.GetHtml(Url: string): string; // starts with 'www.'
var
  hOpen, hUrl: HINTERNET;
  buff: array[0..4095] of Char;
  buffLen: DWORD;
  i: Integer;
begin
  hOpen := InternetOpen(PChar('SeriesBrowser'), INTERNET_OPEN_TYPE_PRECONFIG, nil, nil, 0);
  try
    hUrl := InternetOpenUrl(hOpen, PChar(Url), nil, 0, 0, 0);
    try
      repeat
        InternetReadFile(hURL, @buff[0], SizeOf(buff), buffLen);
        if buffLen = SizeOf(buff) then
          result := result + string(buff)
        else if buffLen > 0 then
          for i := 0 to buffLen - 1 do
            result := result + string(buff[i]);
      until buffLen = 0;
    finally
      InternetCloseHandle(hUrl);
    end;
  finally
    InternetCloseHandle(hOpen);
  end;
end;

procedure TSeriesThread.Execute;
begin
  Synchronize(DoHtml);
end;

procedure TSeriesThread.Login(User: string; Pwd: string);
begin
  FUser := User;
  FPwd := Pwd;
end;

procedure TSeriesThread.DoHtml;
begin
  if Assigned(FOnHtml) then
    FOnHtml(self);
end;

procedure TSeriesThread.SetTitle(Value: string);
begin
  FTitle := Value;
end;

//  < /TSeriesThread >

end.

Das Ziel ist:

Delphi-Quellcode:

private
  FTest: TSeriesThread;
  procedure MyTest(Sender: TObject);

procedure TForm1.MyTest(Sender: TObject);
begin
  // do login
  // search by title
  // get html
  // parse html
  // loop
  // --> FTest.Episodes.Add('episode0... url');
end;

procedure TForm1.Button1Click(Sender: TObject);
begin
  FTest := TSeriesThread.Create(false, MyTest);
  try
    FTest.FreeOnTerminate := false;
    FTest.Resume;
    ListBox1.Items.AddStrings(FTest.Episodes);
  finally
    FTest.Free;
  end;
end;

Die Liste und das Title bleiben leer. Das ist das Problem.

[Blup]

Offensichtlich ist dein Ziel, Aufgaben im Hintergrund abzuwickeln, wärend die Oberfläche weiterhin bedienbar bleibt.
Der Gedanke ist die eigentliche Aufgabe durch einem zusätzlichen Thread auszuführen.
Der braucht aber zum Ausführen der Aufgabe ebenfalls einige Zeit.
Deshalb kann das Ergebnis nicht direkt nach dem Resume abgerufen werden.

Es gibt zwei Möglichkeiten:

1. Warten bis der Thread beendet ist und danach den Hauptthread fortsetzen, um das Ergebnis auszugeben.
Das ist offensichtlich nicht sehr sinnvoll, da dann der Hauptthread in der Zwischenzeit nicht reagiert und die Oberfläche nicht bedienbar ist.
(Man kann das umgehen, aber das ist auch nicht sinnvoll.)

2. Man übergibt dem Thread eine Methode, die automatisch beim Beenden ausgeführt wird.
Der Hauptthread läuft in der Zwischenzeit weiter und kann sich um die Oberfläche kümmern.

Delphi-Quellcode:

procedure TForm1.Button1Click(Sender: TObject);
var
  Test: TSeriesThread;
begin
  Button1.Enabled := False;

  Test := TSeriesThread.Create(false);
  Test.OnTerminate := DoOnTerminate;
  Test.FreeOnTerminate := True;
  Test.Resume;
end; 

procedure TForm1.DoOnTerminate(Sender: TObject);
begin
  ListBox1.Items.AddStrings((Sender as TSeriesThread).Episodes);

  Button1.Enabled := True;
end;

Synchronize wird nur benötigt, wenn der Thread eine Methode aufruft, die auf die VCL zugreift (z.B. Form1.ListBox1 ).
Damit kann man diese Aufgabe an den Hauptthread übergeben. Der Thread wartet dann bis der Hauptthread Zeit hat diese auszuführen.
Im OnTerminate ist der Thread aber bereits beendet. Dieses Ereignis führt der Hauptthread aus und benötigt kein extra Synchronize.
Solche Methoden muss man natürlich so kurz wie möglich halten und möglichst selten aufrufen.
Andernfalls ist der Hauptthread wieder blockiert, was wir gerade vermeiden wollen.

Deshalb gehört alles was in TForm1.MyTest steht, direkt in TSeriesThread.Execute (oder TSeriesThread private Methoden die dort aufgerufen werden).
Falls es mehrere verschiedene Abläufe gibt, die schon beim Start des Thread feststehen, leite für jeden eine Klasse mit eignem Execute ab und führe die konkrete Klasse aus.
Den internen Ablauf über einen Parameter zu steuern ist auch ok.
Deine Variante mit einer Methode, die im Thread ausgeführt, aber außerhalb deklariert ist, birgt immer das Risiko das ohne Synchronize auf die VCL zugegriffen wird.

Delphi-Quellcode:

procedure TSeriesThread.Execute;
begin
  Title := 'Test';
  Episodes.Add('Test Episode1');
  Episodes.Add('Test Episode2');
end;

Zum Schluss sollte man auch verhindern, dass das Formular beendet wird, bevor der TSeriesThread beendet ist:

Delphi-Quellcode:

procedure TForm1.FormCloseQuery(Sender: TObject; var CanClose: Boolean);
begin
  CanClose := Button1.Enabled;
end;

[Kas Ob.]

@Inspur1, hi

Your posted code is wrong, it has many misconceptions, wrong ideas and usage, i am sorry can't point every one right this minute, but will point few as fast as i can as i am left with less than half hour before panned blackout:

1) The most important thing, is that never ever use CheckSynchronize, just don't, this have one place to be used and it is with non-GUI applications.
2) I think it already mentioned above, you are created a thread with CreateSuspended := False; and then after that you go with Resume, your Thread may be is already done and finished before reaching Resume.
3) By using Synchronize in Execute, you defeated the whole point of using threads in the first place as everything within Synchronize will be executed in the main thread blocking your GUI, use Synchronize only to call notifying event, and call GetHtml directly from Execute.
4) I don't understand the following and its logic:

Delphi-Quellcode:

procedure TForm1.MyTest(Sender: TObject);
begin
  // do login
  // search by title
  // get html
  // parse html
  // loop
  // --> FTest.Episodes.Add('episode0... url');
end;

procedure TForm1.Button1Click(Sender: TObject);
begin
  FTest := TSeriesThread.Create(false, MyTest);
  try
    FTest.FreeOnTerminate := false;
    FTest.Resume;
    ListBox1.Items.AddStrings(FTest.Episodes);
  finally
    FTest.Free;
  end;
end;

Notice not every event in your (GUI) forms does need Synchronize, you can use directly call notify from a thread to an event on a form, that is safe and normal, BUT your can't use any GUI element, meaning you can perform the following without Synchronize

Zitat:

// do login
// search by title
// get html
// parse html
// loop

But you can't do this without Synchronize or any other Synchronization method like PostMessage/SendMessage...

Zitat:

// --> FTest.Episodes.Add('episode0... url');

5) Your buffer of 4k is way too small.
6) Your are adding chars one by one !, this is very inefficient and slow.
7) i suggest to change that download function to grab TBytes, and overload it with one that convert the TBytes into string.
...

Hope that helps.