Threadklasse mit Event aktualisiert nicht

[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.