TThreadList - So richtig?

[Björn Ole]

Heyho,

ich benutze gerade zum ersten Mal eine ThreadList und wollte fragen, ob ich damit richtig umgehe. Szenario: Ich habe einen TItemThread, der auf Items aus dem Hauptthread wartet. Der Hauptthread kann über TItemThread.AddItem beliebig viele Items der Liste hinzufügen und anschließend TItemThread.WorkItems aufrufen, damit die Items abgearbeitet werden.

Jetzt bin ich mir aber noch unsicher, was passiert, wenn AddItem und WorkItems aufgerufen wird, während der Thread noch am abarbeiten alter Items ist.

Aber ich poste am besten mal den Code:

Delphi-Quellcode:

{ MainForm }

type
  TForm1 = class(TForm)
    Button1: TButton;
    procedure FormCreate(Sender: TObject);
    procedure FormDestroy(Sender: TObject);
    procedure Button1Click(Sender: TObject);
  private
    FItemThread: TItemThread;
  end;

{...}

procedure TForm1.FormCreate(Sender: TObject);
begin
  FItemThread := TItemThread.Create(false);
end;

procedure TForm1.FormDestroy(Sender: TObject);
begin
  FItemThread.Free;
end;

procedure TForm1.Button1Click(Sender: TObject);
begin
  FItemThread.AddItem('"DatenDaten"');
  FItemThread.AddItem('"DatenDatenDaten"');
  FItemThread.WorkItems;
  FItemThread.AddItem('"DatenDatenDatenDatenDaten"');
  FItemThread.AddItem('"Daten"');
  FItemThread.AddItem('"Daten"');
  FItemThread.WorkItems;
end;

Delphi-Quellcode:

{ ItemThread }

type
  TItemThread = class(TThread)
  private
    FItems: TThreadList;
    FWorkEvent: THandle;
    FTerminateEvent: THandle;
  public
    constructor Create(CreateSuspended: boolean);
    destructor Destroy; override;
    procedure AddItem(const AItem: string);
    procedure WorkItems;
    procedure Execute; override;
  end;

{...}

constructor TItemThread.Create(CreateSuspended: boolean);
begin
  inherited;
  FItems := TThreadList.Create;
  FWorkEvent := CreateEvent(nil, true, false, nil);
  FTerminateEvent := CreateEvent(nil, true, false, nil);
end;

destructor TItemThread.Destroy;
begin
  FItems.Free;
  SetEvent(FTerminateEvent); // WaitForMultipleObjects aufwecken und while-Schleife verlassen
  CloseHandle(FWorkEvent);
  CloseHandle(FTerminateEvent);
  inherited;
end;

procedure TItemThread.AddItem(const AItem: string);
var
  List: TList;
begin
  List := FItems.LockList;
  try
    List.Add(PChar(AItem));
  finally
    FItems.UnlockList;
  end;
end;

procedure TItemThread.WorkItems;
begin
  // Thread benachrichtigen, dass Liste abgearbeitet werden soll
  SetEvent(FWorkEvent);
end;

procedure TItemThread.Execute;
var
  Events: Array[0..1] of THandle;
  bListEmpty: boolean;
  List: TList;
  Item: PChar;
begin
  Events[0] := FWorkEvent;
  Events[1] := FTerminateEvent;
  while not Terminated do
  begin
    case WaitForMultipleObjects(Length(Events), @Events, false, INFINITE) - WAIT_OBJECT_0 of
      0: // Liste abarbeiten
        begin
          repeat
            // erstes Item holen und aus der Liste löschen
            List := FItems.LockList;
            try
              Item := List[0];
              List.Delete(0);
              bListEmpty := List.Count = 0;
            finally
              FItems.UnlockList;
            end;
            Sleep(500); // Mit dem Item arbeiten
            Synchronize( { GUI aktualisieren } );
          until Terminated or bListEmpty; // ...bis beendet oder Liste leer
          ResetEvent(FWorkEvent); // Event für WaitForMultipleObjects wieder zurücksetzen
        end;
      1: // Beenden
        begin
          Terminate;
        end;
    end;
  end;
end;

Mir geht es insbesondere um die Execute Methode. Kann da was schieflaufen?
Wird der Thread in jedem Fall sauber beendet?
Und stimmt es, dass es einen Deadlock geben kann,
wenn ich in dem repeat..until ein Synchronize aufrufe?


Danke, Björn

[samso]

Bei der Threadliste kann ich keinen Fehler entdecken.
In der Execute-Methode würde ich noch einbauen:

Delphi-Quellcode:

if not Terminated then
  Synchronize( { GUI aktualisieren } );

Wenn die Anwendung beendet wird, besteht sonst die Gefahr, dass das Synchronize ewig auf den Hauptthread wartet, während dieser auf das Beenden des Threads wartet.
Vorschlag zum Beenden des Threads:

Delphi-Quellcode:

destructor TItemThread.Destroy;
begin
  Terminated;  //Boolean setzen
  SetEvent(FTerminateEvent); // WaitForMultipleObjects aufwecken und while-Schleife verlassen
  inherited; //Führt WaitFor aus
  //Jetzt ist die Execute-Methode beendet und die internen Variablen können freigegeben werden
  FItems.Free;
  CloseHandle(FWorkEvent);
  CloseHandle(FTerminateEvent);
end;

>"Jetzt bin ich mir aber noch unsicher, was passiert, wenn AddItem und WorkItems aufgerufen wird, während der Thread noch am abarbeiten alter Items ist."

Der kritische Punkt ist das letzte Item. Der Thread glaubt die Liste sei nun leer und bearbeitet den letzten Eintrag. Jetzt wird ein neues Element hinzugefügt und der Event gesetzt (der noch gesetzt ist). Jetzt ist der Thread fertig mit dem vermeintlich letzten Element. Der Event wird zurückgesetzt - und - nichts, das neue Item wird nicht abgearbeitet. Deshalb würde ich die Events automatisch zurücksetzen lassen. Dann würde der Thread im o.a. Szenario sofort wieder einen gesetzen Event sehen und die Liste weiter abarbeiten.
Im anderen Fall - Event wurde nochmals gesetzt, während der Thread arbeitet, würde der Thread sinnlos nochmal prüfen, ob die Liste leer ist. Das ist aber leicht zu verschmerzen. Ups, da fällt mir auf, dieser Fall wird ja nicht abgefangen. Ok, das sollte man noch ändern:

Delphi-Quellcode:

            List := FItems.LockList;
            try
              bListEmpty := List.Count = 0;
              if not bListEmpty then
              begin
                Item := List[0];
                List.Delete(0);
              end;
            finally
              FItems.UnlockList;
            end;
            if not bListEmpty then
            begin
              Sleep(500); // Mit dem Item arbeiten
            end;

Und jetzt, könnte man eigentlich auch auf einen der Aufweck-Events verzichten...

[Björn Ole]

Danke, das hat geholfen. So schauts jetzt aus:

Delphi-Quellcode:

FWakeUpEvent := CreateEvent(nil, false, false, nil);

{...}

procedure TItemThread.Execute;
var
  List: TList;
  Item: PChar;
begin
  while not Terminated do
  begin
    if WaitForMultipleObjects(1, @FWakeUpEvent, false, INFINITE)
      - WAIT_OBJECT_0 = 0 then
    begin
      repeat
        Item := nil;
        List := FItems.LockList;
        try
          if List.Count > 0 then
          begin
            Item := List[0];
            List.Delete(0);
          end;
        finally
          FItems.UnlockList;
        end;
        if (not Terminated) and Assigned(Item) then
        begin
          Sleep(500); // mit dem Item arbeiten
          if not Terminated then
            Synchronize( { GUI aktualisieren } );
        end;
      until Terminated or not Assigned(Item);
    end;
  end;
end;

[jensw_2000]

Ich würde anstatt das FWorkEvent-Handle als Semaphore erstellen.

Das Event hat hier den Nachteil, dass alle WorkerThreads losrennen, wenn das Event gesetzt wurde.
Ist die Anzahl der Jobs kleiner als die Anzahl der WorkerThreads, dann machen einige WorkerThreads eine sinnlose "Leerfahrt".

Bei einer Semaphore rennen nur so viele Threads los, wie nötig.

Du könntest Dir dazu ganz einfach 2 passende Methoden zum Hinzufügen von Jobs basteln.

Delphi-Quellcode:

procedure AddJob(const aJob:String);
begin
  CriticalSection.Enter;
  try
    JobList.add(aJob);
    ReleaseSemaphore(FWorkEvent,1,nil);
  finally
    CriticalSection.leave;
  end;
end; 

end;

procedure AddJobList(const aLobList:TStringList);
  var i:integr;
begin
  if not assigned(aJobList) then exit;
  if not aJobList.Count = 0 then exit;  // damit wir keine "0" Semaphore setzen >> gibt Exception

  CriticalSection.Enter;
  try
    for i:= 0 to aJoblist.Count -1 do
    begin
    JobList.add(aJobList[i]);
    end; 
    ReleaseSemaphore(FWorkEvent,aJoblist.Count,nil);
  finally
    CriticalSection.leave;
  end;
end;

So stellt du sicher, dass der "Count" seiner Semaphore genau so groß ist wie die Anzahl Jobs in der Jobliste.
Da kein WorkerThread losläuft, wenn der Count der Semaphore = 0 ist hast du die "Leerfahrten" der übrigen WorkerThreads gespart...

Grüße
Jens